[PATCH] gdb: remove unneeded value_address call in value_cast_structs

[Simon Marchi <simon.marchi@polymtl.ca>]

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