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

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

[Stephan Rohr]

From: "Rohr, Stephan" <stephan.rohr@intel.com>

Hi all,

this is the second version of my patch.  Changes since v1:

  * Remove the 'internal_error' call in 'rw_pieced_value' if an object's
  'type' and 'enclosing_type' are  not idential.
  * Added additional test cases.

The discussion for v1 can be found at:
https://sourceware.org/pipermail/gdb-patches/2022-October/192547.html

I appreciate your feedback.

Thanks
Stephan

Rohr, Stephan (1):
  gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

 gdb/dwarf2/expr.c                       |  3 --
 gdb/testsuite/gdb.dwarf2/shortpiece.exp | 50 ++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.25.1

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

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

[Stephan Rohr]

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.

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"));
       if (check_optimized)
 	v_contents = nullptr;
       else
diff --git a/gdb/testsuite/gdb.dwarf2/shortpiece.exp b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
index f5a933e521b..f1808bff5be 100644
--- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
+++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
@@ -29,7 +29,7 @@ Dwarf::assemble $asm_file {
 
     cu { addr_size 4 } {
 	compile_unit {} {
-	    declare_labels int_label ushort_label struct_label
+	    declare_labels int_label ushort_label struct_label struct_label_2
 
 	    int_label: DW_TAG_base_type {
 		{DW_AT_byte_size 4 DW_FORM_udata}
@@ -59,6 +59,23 @@ Dwarf::assemble $asm_file {
 		}
 	    }
 
+	    struct_label_2: DW_TAG_structure_type {
+		{DW_AT_name "S_2"}
+		{DW_AT_byte_size 6 DW_FORM_udata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_data_member_location 0 DW_FORM_udata}
+		}
+
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :${ushort_label}}
+		    {DW_AT_data_member_location 4 DW_FORM_udata}
+		}
+	    }
+
 	    DW_TAG_variable {
 		{DW_AT_name "s1"}
 		{DW_AT_type :${struct_label}}
@@ -86,6 +103,20 @@ Dwarf::assemble $asm_file {
 		    DW_OP_piece 8
 		} SPECIAL_expr}
 	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "s3"}
+		{DW_AT_type :${struct_label_2}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {
+		    DW_OP_constu 0
+		    DW_OP_stack_value
+		    DW_OP_piece 4
+		    DW_OP_constu 1
+		    DW_OP_stack_value
+		    DW_OP_piece 2
+		} SPECIAL_expr}
+	    }
 	}
     }
 }
@@ -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\\}"
-- 
2.25.1

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

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

[Rohr, Stephan]

Hi all,

I would like to ask for feedback regarding my patch.

Thank you
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Tuesday, October 25, 2022 3:42 PM
> To: gdb-patches@sourceware.org
> Cc: tom@tromey.com; simark@simark.ca; Rohr, Stephan
> <stephan.rohr@intel.com>
> Subject: [PATCH v2 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted
> to different type.
> 
> 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.
> 
> 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"));
>        if (check_optimized)
>  	v_contents = nullptr;
>        else
> diff --git a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> index f5a933e521b..f1808bff5be 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -29,7 +29,7 @@ Dwarf::assemble $asm_file {
> 
>      cu { addr_size 4 } {
>  	compile_unit {} {
> -	    declare_labels int_label ushort_label struct_label
> +	    declare_labels int_label ushort_label struct_label struct_label_2
> 
>  	    int_label: DW_TAG_base_type {
>  		{DW_AT_byte_size 4 DW_FORM_udata}
> @@ -59,6 +59,23 @@ Dwarf::assemble $asm_file {
>  		}
>  	    }
> 
> +	    struct_label_2: DW_TAG_structure_type {
> +		{DW_AT_name "S_2"}
> +		{DW_AT_byte_size 6 DW_FORM_udata}
> +	    } {
> +		DW_TAG_member {
> +		    {DW_AT_name "a"}
> +		    {DW_AT_type :${int_label}}
> +		    {DW_AT_data_member_location 0 DW_FORM_udata}
> +		}
> +
> +		DW_TAG_member {
> +		    {DW_AT_name "b"}
> +		    {DW_AT_type :${ushort_label}}
> +		    {DW_AT_data_member_location 4 DW_FORM_udata}
> +		}
> +	    }
> +
>  	    DW_TAG_variable {
>  		{DW_AT_name "s1"}
>  		{DW_AT_type :${struct_label}}
> @@ -86,6 +103,20 @@ Dwarf::assemble $asm_file {
>  		    DW_OP_piece 8
>  		} SPECIAL_expr}
>  	    }
> +
> +	    DW_TAG_variable {
> +		{DW_AT_name "s3"}
> +		{DW_AT_type :${struct_label_2}}
> +		{DW_AT_external 1 DW_FORM_flag}
> +		{DW_AT_location {
> +		    DW_OP_constu 0
> +		    DW_OP_stack_value
> +		    DW_OP_piece 4
> +		    DW_OP_constu 1
> +		    DW_OP_stack_value
> +		    DW_OP_piece 2
> +		} SPECIAL_expr}
> +	    }
>  	}
>      }
>  }
> @@ -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\\}"
> --
> 2.25.1

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

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

[Simon Marchi]

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

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

[Rohr, Stephan]

Hi Simon,

thanks for the review, I will fix the indentation and the matching of the
warning with v3 of my patch.

If my understanding is correct, your concerns are related to the cast
itself (as done in value_cast) and this would be a separate patch?

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

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

[Rohr, Stephan]

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