[PATCH] gdb: remove unneeded value_address call in value_cast_structs

[PATCH] gdb: remove unneeded value_address call in value_cast_structs

[Simon Marchi via Gdb-patches]

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);
 
-	  addr2 -= value_address (v) + value_embedded_offset (v);
+	  addr2 -= value_embedded_offset (v);
 	  return value_at (type, addr2);
 	}
     }

base-commit: bc85f56bfda7162688b9bea8dd942ec473bdb21b
-- 
2.35.1

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> -	  addr2 -= value_address (v) + value_embedded_offset (v);
Simon> +	  addr2 -= value_embedded_offset (v);

Thank you.  I appreciated your long commentary.
I think this is ok, especially if it passed the gdb.cp tests :)

Sometimes I wonder if we could get rid of all this "embedded offset"
stuff.  It's an endless source of confusion, at least for me.

Tom

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

[Hannes Domani via Gdb-patches]

 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