RE: [PATCH v2 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

["Rohr, Stephan" <stephan.rohr@intel.com>]

Hi Simon,
Hi Tom,

I submitted version 3 of my patch with the following fixes:
  * Corrected indentation of the last three bullets.
  * Modified test to also check for the warning.

Thanks
stephan

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Thursday, November 10, 2022 4:07 PM
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Cc: tom@tromey.com
> Subject: Re: [PATCH v2 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values
> casted to different type.
> 
> 
> 
> On 10/25/22 09:42, Stephan Rohr via Gdb-patches wrote:
> > From: "Rohr, Stephan" <stephan.rohr@intel.com>
> >
> > The 'rw_pieced_value' function is executed when fetching a (lazy)
> > variable described by 'DW_OP_piece' or 'DW_OP_bit_piece'.  The
> > function checks the 'type' and 'enclosing_type' fields of the value
> > for identity.
> >
> >   * The 'type' field describes the type of a value.
> >   * In most cases, the 'enclosing_type' field is identical to the
> >     'type' field.
> >   * Scenarios where the 'type' and 'enclosing_type' of an object
> >     differ are described in 'gdb/value.c'.  Possible cases are:
> >   * If a value represents a C++ object, then the 'type' field
> >     gives the object's compile-time type.  If the object actually
> >     belongs to some class derived from `type', perhaps with other
> >     base classes and additional members, then `type' is just a
> >     subobject of the real thing, and the full object is probably
> >     larger than `type' would suggest.
> >   * If 'type' is a dynamic class (i.e. one with a vtable), then GDB
> >     can actually determine the object's run-time type by looking at
> >     the run-time type information in the vtable.  GDB may then elect
> >     to read the entire object.
> >   * If the user casts a variable to a different type
> >     (e.g. 'print (<type> []) <variable>'), the value's type is
> >     updated before reading the value.
> 
> I think you should indent the last 3 bullets.
> 
> >
> > If a lazy value is fetched, GDB allocates space based on the enclosing
> > type's length and typically reads the 'full' object.  This is not
> > implemented for pieced values and causes an internal error if 'type'
> > and 'enclosing_type' of a value are not identical.
> >
> > However, GDB can read the value based on its type.  Thus, this patch
> > fixes the previously mentioned cases by removing the check for identity.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28605
> >
> > gdb/ChangeLog:
> > 2022-04-13  Stephan Rohr  <stephan.rohr@intel.com>
> >
> > 	* dwarf2/loc.c (rw_pieced_value): Fix check on 'type' and
> > 	'enlcosing_type' when reading pieced value 'v'.
> >
> > gdb/testsuite/ChangeLog:
> > 2022-04-13  Stephan Rohr  <stephan.rohr@intel.com>
> >
> > 	* gdb.dwarf2/shortpiece.exp: Added test cases.
> > ---
> >  gdb/dwarf2/expr.c                       |  3 --
> >  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 50
> > ++++++++++++++++++++++++-
> >  2 files changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> > 73dfd4b4ffb..fa08ad89024 100644
> > --- a/gdb/dwarf2/expr.c
> > +++ b/gdb/dwarf2/expr.c
> > @@ -161,9 +161,6 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
> >      }
> >    else
> >      {
> > -      if (value_type (v) != value_enclosing_type (v))
> > -	internal_error (_("Should not be able to create a lazy value with "
> > -			  "an enclosing type"));
> 
> I'm still wondering about whether value_cast should change the enclosing
> type.
> 
> When you do:
> 
>     p (int []) s1
> 
> The original value has:
> 
>  - type = struct S
>  - enclosing_type = struct S
>  - embedded_offset = 0
> 
> This means the object we're looking at is of type struct S, and it's at offset 0
> of a top-level containing object of type struct S (itself, in this trivial case).
> After the case, we have:
> 
>  - type = int[2]
>  - enclosing_type = struct S
>  - embedded_offset = 0
> 
> This means the object we're looking at is of type int[2], and it,s at offset 0 of
> a top-level containing object of type struct S.  I don't think that's a wrong way
> of looking at it.  But it's perhaps not that useful either.
> 
> 
> I'm wondering what happens if the value we cast has a different type and
> enclosing_type to begin with.  I imagine the following test case:
> 
>   struct Base1
>   {
>     virtual ~Base1() = default;
>     int a = 1;
>     int b = 2;
>   };
> 
>   struct Base2
>   {
>     virtual ~Base2() = default;
>     int c = 3;
>     int d = 4;
>   };
> 
>   struct Derived : public Base1, Base2
>   {
>     virtual ~Derived() = default;
>     int x = 98;
>     int z = 99;
>   };
> 
>   int main()
>   {
>     Derived d;
>     Base1 *b1 = &d;
>     Base2 *b2 = &d;
>     return 0;
>   }
> 
> In GDB:
> 
>   (gdb) print (int []) *b2
> 
> Before the cast we have:
> 
>  - type = Base2
>  - enclosing_type = Derived
>  - embedded_offset = 16
> 
> After the cast:
> 
>  - type = int[4] (4, not 2, because of the vtable)
>  - enclosing_type = Derived
>  - embedded_offset = 16
> 
> This tells us we have an object of type int[4], at offset 16 within a containing
> object of type Derived.  Again, not a wrong way to look at it.  It's just that the
> type -> enclosing_type relationship seems useless now that the value is cast
> to a type that does not involve inheritance anymore.
> 
> But this shows that if we make value_cast set the enclosing_type, it should
> also reset the embedded_offset to 0.
> 
> Anyway, it's still not clear to me what the right answers are.  If Tom is happy
> with the patch, I am happy with it too.
> 
> > @@ -98,3 +129,20 @@ if { [prepare_for_testing "failed to prepare"
> > ${testfile} \  gdb_test "p s1" " = {a = 1, b = 0}"
> >  gdb_test "p s2" \
> >      "access outside bounds of object referenced via synthetic pointer"
> > +
> > +# When fetching a lazy value, GDB typically tries to fetch the 'full'
> > +# object based on the enclosing type.  GDB does not support the
> > +reading # of a pieced value with a (possibly larger) enclosing type.
> > +However, # the user may want to print a value casted to a different
> > +type, # e.g. print (<type> []) <variable>.  This cast causes an
> > +update of the # value's type.  In case of a pieced value, GDB failed
> > +to fetch the # value's content.
> > +# This test verifies that GDB can print a pieced value casted to a #
> > +different type.
> > +gdb_test "p (int \[\]) s1" " = \\{1\\, 0\\}"
> > +gdb_test "p (short \[\]) s1" " = \\{1\\, 0\\, 0\\, <synthetic pointer>\\}"
> > +
> > +# Test for correct output if the size of the original object is not a
> > +# multiple of the array's element size.
> > +gdb_test "p s3" " = {a = 0, b = 1}"
> > +gdb_test "p (int \[\]) s3" " = \\{0\\}"
> 
> I think you should match the warning here.  Since it's the behavior we expect
> from GDB, we want to make sure there's no regression that causes this
> warning to not be printed.  It's not tested at all elsewhere from what I can
> see.
> 
> Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928