Re: [PATCH] gdb: remove unneeded value_address call in value_cast_structs

[Hannes Domani <ssbssa@yahoo.de>]

 Am Mittwoch, 16. Februar 2022, 04:53:14 MEZ hat Simon Marchi <simark@simark.ca> 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.  We add the
> 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).  This tripped on the code changed by
> this patch, as the types didn't work out.  That lead me to believe that
> the code removed here might be wrong.
>
> That code is used when casting a base class pointer to a derived class
> pointer.  For example, with:
>
>     struct Base1 { int b1; };
>     struct Base2 { int b2; };
>     struct Derived : public Base1, Base2 { int d; };
>
>     int main()
>     {
>       Derived d;
>       return 0;
>     }
>
> and this command:
>
>     (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.  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.  And since value `v` is
> derived from this call just above:
>
>       v = search_struct_field (t2->name (),
>                    value_zero (t1, not_lval), t1, 1);
>
> which looks up a field from a not_lval value, `v` will always be
> not_lval.  And therefore `value_address (v)` will always return 0.  And
> therefore it can be removed.
>
> I did some archeological research to understand what's happening here.
> You can ignore this if not interested.  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).  But 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.  So what this code did was to pretend
> there was a derived object at address 0 and calculate the location of
> the base object.  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.  This is confirmed by these this
> message:
>
>     https://sourceware.org/pipermail/gdb-patches/2002-August/019240.html
>
>     Right --- when casting from a base class to a derived class, we use
>     search_struct_field to find the offset the base class has in the
>     derived class by creating an imaginary instance of the derived class
>     at address zero, finding the base class in that, and then using the
>     base class subobject's address as the offset of the base class in the
>     derived class.
>
> Although the trick is explained in this post, it actually comes from an
> old HP merge commit, here, without more explanation:
>
>   https://gitlab.com/gnutools/binutils-gdb/-/commit/4ef1f4677390c085543fe80eec41b0fe5d58ddca?page=4#4fad73ebb501f19c2b94682df5910fd5c6553d9c_258_332
>
> I tried browsing that version of the code, but there are still gaps in
> my comprehension.  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
> ---
>  gdb/valops.c | 2 +-
>  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)
>        /* Downcasting is possible (t1 is superclass of v2).  */
>        CORE_ADDR addr2 = value_address (v2);


Since you're already here, wouldn't the following fix PR28907?:

-         CORE_ADDR addr2 = value_address (v2);
+         CORE_ADDR addr2 = value_address (v2) + value_embedded_offset (v2);


Regards
Hannes