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

[PATCH v3 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 version 3 of my patch and implements the following
changes on top of version 2:
  * Corrected indentation for git commit message.
  * Extended testcase to also test if warning is emitted.


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 | 53 ++++++++++++++++++++++++-
 2 files changed, 52 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 v3 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 | 53 ++++++++++++++++++++++++-
 2 files changed, 52 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..6f45887a881 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,23 @@ 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" [multi_line \
+				"warning: array element type size does not divide object size in cast" \
+				"\\$\\d = \\{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 v3 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Tom Tromey]

>>>>> Stephan Rohr via Gdb-patches <gdb-patches@sourceware.org> writes:

> 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.

Thanks for the patch.

> 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.

I thought there was some other discussion & idea about the cause of this
patch... something like, setting a value's type should reset the
enclosing type?  Or vice versa?  I am wondering if you tried this
approach instead.

Tom

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

[Simon Marchi]

On 1/5/23 14:52, Tom Tromey wrote:
>>>>>> Stephan Rohr via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> 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.
> 
> Thanks for the patch.
> 
>> 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.
> 
> I thought there was some other discussion & idea about the cause of this
> patch... something like, setting a value's type should reset the
> enclosing type?  Or vice versa?  I am wondering if you tried this
> approach instead.

For reference, here was my last attempt at understanding what happens:

  https://inbox.sourceware.org/gdb-patches/dd04b3b4-ea6a-07ba-d734-0e542ca136b3@simark.ca/

I don't have a strong opinion on the matter, I don't know if one
solution is more corrent than the other.  The thing is, I can't explain
the logic behind the existing assert.  It's missing a comment explaining
why it would be a bad thing to "create a lazy value with an enclosing
type".  If we can't add such a justification, I think it's ok to remove
it...

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I don't have a strong opinion on the matter, I don't know if one
Simon> solution is more corrent than the other.  The thing is, I can't explain
Simon> the logic behind the existing assert.  It's missing a comment explaining
Simon> why it would be a bad thing to "create a lazy value with an enclosing
Simon> type".  If we can't add such a justification, I think it's ok to remove
Simon> it...

I think the idea is that the enclosing type stuff is all there for "set
print object on" -- and so for a value to have an enclosing type, it
must already have contents, because the enclosing type is the runtime
type, which can only be determined by reading memory.

Tom

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

[Rohr, Stephan]

Hi Tom.

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Thursday, January 5, 2023 8:53 PM
> To: Stephan Rohr via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Rohr, Stephan <stephan.rohr@intel.com>
> Subject: Re: [PATCH v3 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values
> casted to different type.
> 
> >>>>> Stephan Rohr via Gdb-patches <gdb-patches@sourceware.org>
> writes:
> 
> > 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.
> 
> Thanks for the patch.
> 
> > 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.
> 
> I thought there was some other discussion & idea about the cause of this
> patch... something like, setting a value's type should reset the enclosing
> type?  Or vice versa?  I am wondering if you tried this approach instead.
> 
> Tom

There was a discussion with Simon whether the value cast should update the
enclosing type.  Simon tried it out but as I understood he's also not clear on
what the right answer answers are.  From my point of view, this would also
be a separate patch.

Thanks
Stephan
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 v3 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Tom Tromey]

> There was a discussion with Simon whether the value cast should update the
> enclosing type.  Simon tried it out but as I understood he's also not clear on
> what the right answer answers are.  From my point of view, this would also
> be a separate patch.

Ok, that's fair.
I think the patch is ok.

Tom

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

[Rohr, Stephan]

Hi Tom.

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Monday, January 9, 2023 8:29 PM
> To: Rohr, Stephan via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Tom Tromey <tom@tromey.com>; Rohr, Stephan
> <stephan.rohr@intel.com>; Simon Marchi <simark@simark.ca>
> Subject: Re: [PATCH v3 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values
> casted to different type.
> 
> > There was a discussion with Simon whether the value cast should update
> > the enclosing type.  Simon tried it out but as I understood he's also
> > not clear on what the right answer answers are.  From my point of
> > view, this would also be a separate patch.
> 
> Ok, that's fair.
> I think the patch is ok.
> 
> Tom

Thank you for your review.  This is my first patch.  Can you push it for me?
I don't have any write access.

I plan to upstream more patches in the future and would like to have
"write after approval" access.  Could I put your email address in the field
which says "email address of person who approved request" in the application
form at https://sourceware.org/cgi-bin/pdw/ps_form.cgi ?

Thanks
Stephan
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 v3 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Tom Tromey]

>> Thank you for your review.  This is my first patch.  Can you push it for me?
>> I don't have any write access.

Yep.  I fixed a whitespace error (an extra blank line at the end of the
test file) that git pointed out.

>> I plan to upstream more patches in the future and would like to have
>> "write after approval" access.  Could I put your email address in the field
>> which says "email address of person who approved request" in the application
>> form at https://sourceware.org/cgi-bin/pdw/ps_form.cgi ?

Yes.

Tom