From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic308-18.consmr.mail.ir2.yahoo.com (sonic308-18.consmr.mail.ir2.yahoo.com [77.238.178.146]) by sourceware.org (Postfix) with ESMTPS id 6F980389367A for ; Wed, 29 Apr 2020 16:58:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6F980389367A X-YMail-OSG: TXgsJPoVM1kQkyF_344Df.dOiIT_SxTvVUnvrFJAVKbG1tSn5QPxEBSqt1MeFLR s77WSHteQWAArdKC_jMWg3IEV3E9ZrHN2TD783qJOMIBhBszyGTGwwv9w1hqLUcYGEzjF8jViL93 g5JvuyA720xLWj13zuHzclLVLQXwORtjKHL98XHeHfOJ5r4iqOcs3IrVU7Y.omBEhynrUKehaO_7 G1yME.d_t5zbSXdeYTEq0nq4rD9jCaSjLRMiEZpBj1mHTh8vsZE5qgakbvYf0rymCS3aIiIjLifg yWvD1V724BZ4xVrVq6aPfA4T8dxdlMZlnZR3_5pvTbgZsgAGLr1u3eZsBd4xMm3elMJB0znXsfzg ttCWPNcV5y6i1TwWqhtCQwTiPBvn1l3ZXIZ7MplbfxrXPvyKGDiZcm_nuqEpFidA6THjB3y80a8U KLo5GLqQ.W2b5FXHCskHydanpwtzzi._srbtv8_J113p3zaeEmJHppBRmrS.Vqe8C_HD5sbyl6cD QKZbll1H7ENqWNSCGzdKJo.a6cZBDw9fBghaVO7AVRe08cDsBnEAWXaqDC1WFqTBdUxzUvaQYG21 H9TWvp0FGI1OwLPoOKQtvr2.NeumrfdMv_z017wirgd4EuWy7W95L4TtOp6zJ5Hu3z1PUQTfS8Sl 1xKv3_Ynnc8gX7I30fUDDDDfgeZFdAsTHIsVqCybqdgU6QiGF9w3W12YeV.AsGqRHHreY3zy4RKE LhVtcRsMcL6aDoW.1sA8E67OO4rD9sRD0Ihpybw5YjuTeTBMj00VmyEf9.6CdY8sMCH12A9m1bTQ ljJkKTaQwhbAiWZwDddIRJ85cUgKbaWdUPIipK.TKckwZ3dp0imXMAT0F3l3gUMwJ5FQhuEhBu0T MplFv7wtC4R1hE9nbD6_Dplsm902m3s2LeCxte7y1A9dYek4N6jvSv6M9SKpnlGepa5wQxjyZDbc 2A9e8z2RO2wnWsjQZvGUHk3j9ISbxBdY73mArrq1na.eiD3LJ7S_adNenI9_bdaM1AG2Ng.3rxoh z2ZlDPVvgC5xo6RKfy9gJZqyEtm5ATl.5DW18iPEHcOdW7ps8y.39pijqKg_Se_JrxWDq59dVTeJ 3fZB2Ow8uWIjhhiUHo2uOe8D_GJEQgA67BIA5umuStwzzNIM1a_iBkW0QmIrkRamIwhJXZkwc1dN YYiHSaNeQO8f_gmEQT_16lSd5uOf0BkNYbjmEuEr9EDj9zU.ZltPFAdHOp1G1ULGQZyin9BiyxLa ZARE.43cVRNsLqbF7p8ApBCSXeuVNRzLFEhOBG8j8XxikpEhNCUqS9Cqcqr.KJjlmtGYMel2JRxx Bi_yp9o0StiRFT6dlww-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic308.consmr.mail.ir2.yahoo.com with HTTP; Wed, 29 Apr 2020 16:58:26 +0000 Date: Wed, 29 Apr 2020 16:58:26 +0000 (UTC) From: Hannes Domani To: Gdb-patches Message-ID: <2135104991.4527821.1588179506131@mail.yahoo.com> In-Reply-To: References: <20200427114154.2275-1-ssbssa.ref@yahoo.de> <20200427114154.2275-1-ssbssa@yahoo.de> <82e2e5d4-f5e3-7a89-5bcb-bfe305c601e4@simark.ca> <970532437.4157409.1588164436815@mail.yahoo.com> Subject: Re: [PATCH][PR gdb/18706] Calculate size of array of stubbed type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Mailer: WebService/1.1.15756 YMailNorrin Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0 X-Spam-Status: No, score=-18.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Apr 2020 16:58:29 -0000 Am Mittwoch, 29. April 2020, 18:39:47 MESZ hat Simon Marchi Folgendes geschrieben: > On 2020-04-29 8:47 a.m., Hannes Domani via Gdb-patches wrote: > >=C2=A0 Am Dienstag, 28. April 2020, 17:10:29 MESZ hat Simon Marchi Folgendes geschrieben: > > > >> On 2020-04-27 7:41 a.m., Hannes Domani via Gdb-patches wrote: > >>> Sizes of stubbed types are calculated on demand in check_typedef, so = the same > >>> must also be done for arrays of stubbed types. > >> > >> For the uninitiated, can you please explain what's happening here, whi= ch types > >> are involved, which ones are stubs, and what that means.=C2=A0 That wo= uld help me > >> understand what happens here and be confident that the patch is correc= t. > > > > Something like?: > > > > A stubbed type is usually a structure that has only been forward declar= ed, but > > can also happen if the structure has a virtual function that's not inli= ne in > > the class definition. > > > > For these stubbed types, the size must be recalculated once the full de= finition > > is available. > > I'm mostly interested in understanding how we get there. > > In the CU for stub-array-size.cc, we have: > > 0x0000002d:=C2=A0 DW_TAG_array_type >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_type=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (0x000000= 44 "A") >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_sibling=C2=A0 (0x0000003d) > > which points to > > 0x00000044:=C2=A0 DW_TAG_structure_type >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_name=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ("A") >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_declaration=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (t= rue) > > I understand that this creates a stub type for `struct A`, since we only > see a declaration.=C2=A0 We then create a type that is an array of a stub= type. > So the `struct type` we have created for the array points to a `struct ty= pe` > that has no size information, is that right? > > The other CU, for stub-array-size2.cc, contains an actual definition for > the structure type: > > 0x0000012e:=C2=A0 DW_TAG_structure_type >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_name=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ("A") >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_byte_size (0x08) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_decl_file ("/home/smarchi/src/binutils-gdb/g= db/testsuite/gdb.cp/stub-array-size.h") >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_decl_line (18) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_decl_column=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (0= x08) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_containing_type=C2=A0 (0x0000012e) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 DW_AT_sibling=C2=A0 (0x00000167) > > For your code to work (and it appears to work, I just want to understand)= , > it means that at some point, we changed the target of the array type to > point to a `struct type` that does have size information.=C2=A0 I'm guess= ing > using the information from the other CU.=C2=A0 I'm wondering when/how thi= s happens. It happens a few lines above the array stuff: =C2=A0 if (TYPE_TARGET_STUB (type)) =C2=A0=C2=A0=C2=A0 { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct type *target_type =3D check_typedef (= TYPE_TARGET_TYPE (type)); At this point, the struct has TYPE_STUB set, so check_typedef replaces the stub with the complete type. > >>> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > >>> index 157b3c5e61..5e9de2ccb1 100644 > >>> --- a/gdb/gdbtypes.c > >>> +++ b/gdb/gdbtypes.c > >>> @@ -2637,6 +2637,22 @@ check_typedef (struct type *type) > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TYPE_LENGTH (type) = =3D TYPE_LENGTH (target_type); > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TYPE_TARGET_STUB (typ= e) =3D 0; > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else if (TYPE_CODE (type) =3D=3D TYPE= _CODE_ARRAY > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && TYPE_NFIEL= DS (type) =3D=3D 1) > >> > >> Why is the `TYPE_NFIELDS (type) =3D=3D 1` check necessary here? > > > > This is de define of TYPE_INDEX_TYPE: > > #define TYPE_INDEX_TYPE(type) TYPE_FIELD_TYPE (type, 0) > > > > So I assumed I had to check if the field exists before using it. > > > > This check is not always done (resolve_dynamic_array_or_string), someti= mes > > with gdb_assert (is_dynamic_type_internal), or like I did > > (is_scalar_type_recursive). > > > > I don't know which is the most correct way. > > If it is an invariant that all `struct type` that are TYPE_CODE_ARRAY > have exactly one field (to store the index type), then we don't need > to check it here.=C2=A0 If we stumble on an array type that does not have > one field, it means there's a bug somewhere else in GDB, but this code > here should not try to compensate / work around that. > > It might make sense to assert for for it, to make sure that the invariant > is respected: > >=C2=A0=C2=A0 gdb_assert (TYPE_NFIELDS (type) =3D=3D 1); > > But it would be silly to do this everywhere we use TYPE_INDEX_TYPE. > Instead, I could see this macro being replaced with a `struct type` > method that does it: > > struct type * > type::get_index_type () > { >=C2=A0=C2=A0 gdb_assert (TYPE_NFIELDS (this) =3D=3D 1); > >=C2=A0=C2=A0 return ... > } > > That's of course outside the scope of this patch.=C2=A0 But all this to s= ay > that if the `TYPE_NFIELDS (type) =3D=3D 1` check is only there to check > something that's always supposed to be true, it's not necessary, and/or > should be an assertion. > > If there are valid cases where we have an array type with NFIELDS !=3D 1, > then forget about all this.=C2=A0 But looking at create_array_type_with_s= tride, > I don't think there are. > > >>> +=C2=A0=C2=A0=C2=A0 { > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct type *range_type =3D check_typ= edef (TYPE_INDEX_TYPE (type)); > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (TYPE_CODE (range_type) =3D=3D TYP= E_CODE_RANGE > >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && has_static= _range (TYPE_RANGE_DATA (range_type))) > >> > >> Is it possible to have an array type where the index type is not TYPE_= CODE_RANGE? > >> > >> If it is not, then we should assert `TYPE_CODE (range_type) =3D=3D TYP= E_CODE_RANGE`.=C2=A0 If it > >> can be of another type, what are the other possibilities? > > > > Again here, this check is done differently in various functions. > > > > Either gdb_assert (resolve_dynamic_range), or explicit check > > (is_scalar_type_recursive). > > > > If I see these different approaches, I tend to use the safest myself. > > Same argument as above for NFIELDS. > > It might look silly, but I think it's important to use the right one, bec= ause > as a reader, > >=C2=A0=C2=A0 if (TYPE_CODE (range_type) =3D=3D TYPE_CODE_RANGE) > > and > >=C2=A0=C2=A0 gdb_assert (TYPE_CODE (range_type) =3D=3D TYPE_CODE_RANGE) > > don't communicate the same thing to me.=C2=A0 If we use the first one > when in reality the index types of arrays always are TYPE_CODE_RANGE, > then it makes me continue with the idea that things are more complex > than they are in reality.=C2=A0 Using the second one helps reduce the > cognitive load, because it tells me there aren't other cases to > consider. > > Looking at the various callers of create_array_type_with_stride and > create_array_type, I think that index types are always TYPE_CODE_RANGE. OK, I will remove both the TYPE_NFIELDS and TYPE_CODE_RANGE checks. > >>> diff --git a/gdb/testsuite/gdb.cp/stub-array-size.exp b/gdb/testsuite= /gdb.cp/stub-array-size.exp > >>> new file mode 100644 > >>> index 0000000000..cb486cd459 > >>> --- /dev/null > >>> +++ b/gdb/testsuite/gdb.cp/stub-array-size.exp > >>> @@ -0,0 +1,27 @@ > >>> +# Copyright 2020 Free Software Foundation, Inc. > >>> +# > >>> +# This program is free software; you can redistribute it and/or modi= fy > >>> +# it under the terms of the GNU General Public License as published = by > >>> +# the Free Software Foundation; either version 3 of the License, or > >>> +# (at your option) any later version. > >>> +# > >>> +# This program is distributed in the hope that it will be useful, > >>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of > >>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.=C2=A0 See the > >>> +# GNU General Public License for more details. > >>> +# > >>> +# You should have received a copy of the GNU General Public License > >>> +# along with this program.=C2=A0 If not, see . > >>> + > >>> +# This file is part of the gdb testsuite. > >> > >> > >> Please add a short introductory comment that explains what this intend= s to test. > > > > Like?: > > > > Test size of arrays of stubbed types (structures where the full definit= ion is > > not immediately available). > > Yes, that's perfect, thanks.=C2=A0 It's just enough to save the trouble t= o the reader > to read an interpret the test to know what this is about. Hannes