From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sonic313-21.consmr.mail.ir2.yahoo.com (sonic313-21.consmr.mail.ir2.yahoo.com [77.238.179.188]) by sourceware.org (Postfix) with ESMTPS id EE82C3858D1E for ; Sat, 5 Mar 2022 00:42:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EE82C3858D1E X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1646440927; bh=NHFWC3ILKnSPdLvwXD7JpFFYbRO7DMDu33rcgcGS/GI=; h=X-Sonic-MF:Date:From:To:Subject:From:Subject; b=hp5EoUKQPJ0+tbLmoktW5pbGhmMPq3xpK6DHuA50rI4LBqNJWsaXDJaK1v//v49ZTnw3bz9ex8cOJO7xCwYl7S2YywEHIyyOuBWCoJNilptGIZd8zKvyd+vGh7YGceIS3q5c1Nmzta2bFMub9vqY1oQzRklI1DX22u15UmCLzL8vawxxJumbgM+dhS7tdbqZNIH/RRW4dLQwSr42rtw/hqlqicJs0iiCd0KzhZpoWHjYPv2T6TnXLKuiJg/8H0DNIIpBOK6C4PUSBfDapC3MdOzvI2p7pgiByyWHIt72gKuGByquz77g9qB+fP2S2lkj2UpMG74q9jUXX/1FJAnnOA== X-YMail-OSG: HR6inn0VM1mcxWjweSYQCDC_hotdF99_NH2dVWOGwdBZzOPXYkKbg9r2sQ_P7zZ kFFJpd3BOs6O66.NUlPlQi.EebkgqCTwaYNKDX9L22DuexjfNt2v56TxrWFznb2gRWv5Z8ma.E4R _EDBLWOJOGkBc.ILP1p4kvLYaki8kKnTPXGGha8oNx9zK3mQIR09gOlJiVEyZi4NSs1zBZZyy447 tuyYipCrZyjr1AojZEKQ6NXYsBXZICUCpqwHTfUMaPwyVmQFPhRrHvaG4Ihn9gTq_JFAAd8QivDG CwPb.XwyDnXDRPGr4h2h8zgHx6uRj1km9JE535evc.zeBvYg6qFbIG81EWyT1Ivxp0Bq0RrglX5K 9JDelA.Q3iGsSO2CliFrcJcJly4XZnzpRdT0OvgwyowhGjnGThJIT7CEHUHBQZnKZVtS8AN_U8eF QFKgablt3Ep4eehvt.wevFjE_O99tnOw0GokdLpFKWlSq5e_8UBYbG9ujdNo1UlbUKCo1yMwSzpM KhN3sGmh0mnqSmzaWUQ.pQbAVNa3LdffdryoPsnnIYTdHQvJps6x3JRQvsXiYUDEttu4t9V9T7iq SJxNHajDQQn5n7OvYCWh6v6_8zTKSJ17e.8Fsa9ZhO6ybe0nt8bmTO8TRu0.TQ_nGPoW3mrbcYTp 3Qy2IR7q8io4wd_wvFkc_uWlu4KDzsliT3CSKlh52kWK_Uqk4ghbvKoo07yff.ooeMtbuNcgxV5n 5FxRKHGb_GXQTBy8E80FH_ua9Xf0AzbUYkPuTOy2F.WDmfgevMKhVhhsjXcM3CQLDRfClwkSwxzd jCqFXh8uyn.I4RBYyJQzvpOYvMaL5XykYrqTKJIZVawJWTWzv9iA698YJDO8NHq34peM2bFtOecx Me02deLL9vVUL7r5pnyQ.9IBGB1OtV0tgi4IQQnNVs0p4F6n76tIlEHG40lMwZz9Fp0Qu9sY08Ky .mJLBSIYRTk7xLMWKqc8xBsiiovPMLi2h.cg_M_2A1wb5Ixm6Y53iHNqZ7GYWPiX9kmL7A7VROPU q5ZjVwY38F9QyGZvYOP58QQIHi253AjIOW939rVRaZZH1GvTvWaUlTlgExa2JosbaoS.G7H.T9TK 5fN8mXpC0q2Zz82voNPBJxPT1cE_CULvT2Mbf13RKd_li8CtiqbyzfCNsiqULzTWgjToBFp8IhgK DnPg0OBkIGB4uyaY3pEXCr234oQAAl3Vz3VSNaMZOFqP8WHSqqYpSBirrNkO7CJ2if4X23mBn7Hw ksTee6LC3KoXMQRZBUpXfijMiNDMwjdY09H0QnqRLfHfKhmC39_ipDejxMELFoo4OplRQXiEecRu s563lTjnNB4FpQeuCWHwmn7vqb.PJx9RHMN3TZ3FSEyvFy5.RGYS9QDGcZuw5JMN3i107NtZnCFs OVZ5Sl4TEl82z0UbIfU853Ta0sgn7N9HyVQiVFjVXiWBpNYe4SNsIQcLQQVRAC9Kbn4yjZ.h7A_1 jJZS13wLFzPAkpKxnHE_HtJXZ8kVGds9IGWh3dTY_QWgmQhv0cS3.WANWEm0bOAUtaoquh1SdTz6 OWYAElNFwOwL6eNfyRStu_GtYQYIHL0a.K061cO9EPr.nAkhezFczmpxaUH6Kce_EWIqZL8fJRZQ LOiQ83gsQa8VmCLS6JYxO837HzVxDTn3KXt8NSfLvVloAlaVTUP9oEG5HLEYaOqsYKDW.Rnw5EdX HeHpk.UYZFadmBkepEhdECOkQje.yKatKifqc8DzkE.yyaLlaB2m6ql02r5EyDJmdnFAPIyy298X XxEr2p7C33V4SJUEKLczghS.jbZJkrKKdU_B5nlossmfXVODjFBVx2yl5iRJzUe4RibQhCIVe4nQ wQp6WRGoVBd.YWu8l2l36WedmlcW.3urgXcaFI6AY8j0jXIOFgzMPuMf3j_2_JUisUulWLJE4kvO _Ubt4MkkAdA0GoJzvQgRo_1OT2Rz0kX3K4WFsQGSZGNhxkYAAsvYF..KCTka3syAyWl8rv5Tv3mt h.mg0nXcEPuNBjzTn9.Z0lLx8foxpYlHYYaTrhE_0AmYnwUw9bjR_PGka3_lOM80Sl8skf7jgmW5 nhYq2XRD_U0CzDae5vz8L7sffEaPHi5MWBhDhIRl9kGpEQcmJI7U7maD6gDhIAJ9EB5ZLGuIxw_T 6UVYlHG6NsCZ.kS93lgJT7P2xDKagv7r_d.QyulAsJpHioYZhU75zEivH.WNlGnmROdw.LJZJenW jwMk1Qrz.NcypEyb1EO9qOmPCkKUBtsnPK9EbEQAu5nd.Q74xNLL_MU.UVtgtlCLZ2S9gfVJzKWB 9BJc00yRI4HOIEjLo X-Sonic-MF: Received: from sonic.gate.mail.ne1.yahoo.com by sonic313.consmr.mail.ir2.yahoo.com with HTTP; Sat, 5 Mar 2022 00:42:07 +0000 Date: Sat, 5 Mar 2022 00:42:38 +0000 (UTC) From: Hannes Domani To: Simon Marchi via Gdb-patches , Simon Marchi Message-ID: <1983967698.43432.1646440958753@mail.yahoo.com> In-Reply-To: <87o82lqydx.fsf@tromey.com> References: <20220216045314.813723-1-simon.marchi@polymtl.ca> <87o82lqydx.fsf@tromey.com> Subject: Re: [PATCH] gdb: remove unneeded value_address call in value_cast_structs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Mailer: WebService/1.1.19797 YMailNorrin X-Spam-Status: No, score=-9.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sat, 05 Mar 2022 00:42:11 -0000 Am Mittwoch, 16. Februar 2022, 04:53:14 MEZ hat Simon Marchi Folgendes geschrieben: > In the context of the AMD ROCm port of GDB, in order to better support > multiple address spaces, we are introducing a new type `gdb::address` > that contains a CORE_ADDR plus an address space specifier.=C2=A0 We add t= he > appropriate operator overloads to allow common operations, such as > adding an address and an offset (yielding an address) and subtracting > two addresses (yielding an offset).=C2=A0 This tripped on the code change= d by > this patch, as the types didn't work out.=C2=A0 That lead me to believe t= hat > the code removed here might be wrong. > > That code is used when casting a base class pointer to a derived class > pointer.=C2=A0 For example, with: > >=C2=A0=C2=A0=C2=A0=C2=A0 struct Base1 { int b1; }; >=C2=A0=C2=A0=C2=A0=C2=A0 struct Base2 { int b2; }; >=C2=A0=C2=A0=C2=A0=C2=A0 struct Derived : public Base1, Base2 { int d; }; > >=C2=A0=C2=A0=C2=A0=C2=A0 int main() >=C2=A0=C2=A0=C2=A0=C2=A0 { >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Derived d; >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >=C2=A0=C2=A0=C2=A0=C2=A0 } > > and this command: > >=C2=A0=C2=A0=C2=A0=C2=A0 (gdb) p (Derived *)(Base2 *)&d > > When casting from a `Base2 *` to a `Derived *`, GDB must subtract from > the Base2 pointer value the offset between the start of a `Derived` > object and the start of the Base2 object embedded in it. > > In the code, `addr2` is the value of the Base2 pointer, and `v` is a > value of type `Base2`, with its embedded offset field set to the offset > of a `Base2` object within a `Derived` object. > > So, subtracting `value_embedded_offset (v)` from addr2 makes sense.=C2=A0= But > subtracting `value_address (v)` doesn't seem to make sense. > > Value `v` is of lval kind `not_lval`, so `value_address (v)` returns 0, > so there's no actual harm, it's just useless.=C2=A0 And since value `v` i= s > derived from this call just above: > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 v =3D search_struct_field (t2->name (= ), > =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 value_zero (t1, not_lval), t1, 1); > > which looks up a field from a not_lval value, `v` will always be > not_lval.=C2=A0 And therefore `value_address (v)` will always return 0.= =C2=A0 And > therefore it can be removed. > > I did some archeological research to understand what's happening here. > You can ignore this if not interested.=C2=A0 In today's DWARF, the offset= of > a base classe is described using a constant value for the > DW_AT_data_member_location attribute in the DW_TAG_inheritance DIE (at > least for all compilers I know, there's no reason not to do it).=C2=A0 Bu= t in > DWARF 2 [1], DW_AT_data_member_location could only be a location > description yielding the address of the base object, given the address > of the derived object as input.=C2=A0 So what this code did was to preten= d > there was a derived object at address 0 and calculate the location of > the base object.=C2=A0 That technically yields an address, but since we > started with the derived object at address 0, the resulting address is > equal to the offset we are looking for.=C2=A0 This is confirmed by these = this > message: > >=C2=A0=C2=A0=C2=A0=C2=A0 https://sourceware.org/pipermail/gdb-patches/2002= -August/019240.html > >=C2=A0=C2=A0=C2=A0=C2=A0 Right --- when casting from a base class to a der= ived class, we use >=C2=A0=C2=A0=C2=A0=C2=A0 search_struct_field to find the offset the base c= lass has in the >=C2=A0=C2=A0=C2=A0=C2=A0 derived class by creating an imaginary instance o= f the derived class >=C2=A0=C2=A0=C2=A0=C2=A0 at address zero, finding the base class in that, = and then using the >=C2=A0=C2=A0=C2=A0=C2=A0 base class subobject's address as the offset of t= he base class in the >=C2=A0=C2=A0=C2=A0=C2=A0 derived class. > > Although the trick is explained in this post, it actually comes from an > old HP merge commit, here, without more explanation: > >=C2=A0=C2=A0 https://gitlab.com/gnutools/binutils-gdb/-/commit/4ef1f467739= 0c085543fe80eec41b0fe5d58ddca?page=3D4#4fad73ebb501f19c2b94682df5910fd5c655= 3d9c_258_332 > > I tried browsing that version of the code, but there are still gaps in > my comprehension.=C2=A0 I am not sure where, when calling > search_struct_field, the address of the resulting value would be > adjusted. > > [1] https://dwarfstd.org/doc/dwarf-2.0.0.pdf > > Change-Id: I118c4ffbbff94d54d9fbee3bbb191d9cea4a7481 > --- >=C2=A0 gdb/valops.c | 2 +- >=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/valops.c b/gdb/valops.c > index 3a595125752f..5d9f6ebf004a 100644 > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -276,7 +276,7 @@ value_cast_structs (struct type *type, struct value *= v2) > =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0 /* Downcasting is possible (t1 is supercl= ass of v2).=C2=A0 */ > =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0 CORE_ADDR addr2 =3D value_address (v2); Since you're already here, wouldn't the following fix PR28907?: -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CORE_ADDR addr2 =3D value= _address (v2); +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CORE_ADDR addr2 =3D value= _address (v2) + value_embedded_offset (v2); Regards Hannes