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

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

[Stephan Rohr]

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

This patch provides a fix for the issue when a user wants to print a pieced
value, as described by 'DW_OP_piece' or 'DW_OP_bit_piece', casted to a
different type, e.g. 'print (<type> []) <variable>).

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, which is the case if the user explicitly casts the
variable.

However, GDB can read the value based on its type.  Thus, it should be
sufficient to check if the type's length (potentially shifted by
'embedded_offset') does not exceed the enclosing type's length which was
used for memory allocation.

Test-cases are added to 'gdb/testsuite/gdb.dwarf2/shortpiece.exp'.

The testsuite was run for the following targets:
* unix/-64
* native-gdbserver
* native-extended-gdbserver
* unix/-m32

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

 gdb/dwarf2/expr.c                       |  6 ++----
 gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 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, it should be
sufficient to check if the type's length (potentially shifted by
'embedded_offset') does not exceed the enclosing type's length which was
used for memory allocation.
---
 gdb/dwarf2/expr.c                       |  6 ++----
 gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 99862583336..6330a5787fc 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool check_optimized)
     }
   else
     {
-      if (value_type (v) != value_enclosing_type (v))
-	internal_error (__FILE__, __LINE__,
-			_("Should not be able to create a lazy value with "
-			  "an enclosing type"));
+      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset (v))
+		  <= TYPE_LENGTH (value_enclosing_type (v)));
       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..19cdec83193 100644
--- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
+++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
@@ -98,3 +98,15 @@ 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>\\}"
-- 
2.25.1

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

[Bruno Larsen]

Hello Stephan

Your patch does not seem to introduce any regression and fixes the test, so I'd say it looks alright. Unfortunately, I can't approve patches, so let's hope a global maintainer looks at it.

Cheers!
Bruno Larsen

On 4/22/22 11:44, Stephan Rohr via Gdb-patches wrote:

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

[Rohr, Stephan]

Hi all,

I would like to ask for review of my patch.

Best
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Friday, April 22, 2022 4:44 PM
> To: gdb-patches@sourceware.org
> Cc: Rohr, Stephan <stephan.rohr@intel.com>; tom@tromey.com
> Subject: [PATCH 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.
> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset
> (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));
>        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..19cdec83193 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -98,3 +98,15 @@ 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>\\}"
> --
> 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 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Rohr, Stephan]

Hi All,

I would like to ask for review of my patch.

Best
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Friday, April 22, 2022 4:44 PM
> To: gdb-patches@sourceware.org
> Cc: Rohr, Stephan <stephan.rohr@intel.com>; tom@tromey.com
> Subject: [PATCH 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.
> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset
> (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));
>        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..19cdec83193 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -98,3 +98,15 @@ 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>\\}"
> --
> 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 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Rohr, Stephan]

Hi all,

I would like to ask for review of my patch.

Best
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Friday, April 22, 2022 4:44 PM
> To: gdb-patches@sourceware.org
> Cc: Rohr, Stephan <stephan.rohr@intel.com>; tom@tromey.com
> Subject: [PATCH 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.
> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset
> (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));
>        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..19cdec83193 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -98,3 +98,15 @@ 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>\\}"
> --
> 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 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Rohr, Stephan]

Hi all,

I would like to ask for review of my patch.

Best
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Friday, April 22, 2022 4:44 PM
> To: gdb-patches@sourceware.org
> Cc: Rohr, Stephan <stephan.rohr@intel.com>; tom@tromey.com
> Subject: [PATCH 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.
> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset
> (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));
>        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..19cdec83193 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -98,3 +98,15 @@ 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>\\}"
> --
> 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 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Rohr, Stephan]

Hi All,

I would like to ask for review of my patch.

BR
stephan

> -----Original Message-----
> From: Rohr, Stephan <stephan.rohr@intel.com>
> Sent: Friday, April 22, 2022 4:44 PM
> To: gdb-patches@sourceware.org
> Cc: Rohr, Stephan <stephan.rohr@intel.com>; tom@tromey.com
> Subject: [PATCH 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.
> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset
> (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));
>        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..19cdec83193 100644
> --- a/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> +++ b/gdb/testsuite/gdb.dwarf2/shortpiece.exp
> @@ -98,3 +98,15 @@ 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>\\}"
> --
> 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 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type.

[Simon Marchi]

On 4/22/22 10:44, 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.

Out of curiosity, where do you see these scenarios described?  I could
not find where value.c talks about the case where the user casts the
variable.

>
> 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, it should be
> sufficient to check if the type's length (potentially shifted by
> 'embedded_offset') does not exceed the enclosing type's length which was
> used for memory allocation.

I'm trying to understand a bit better what happens here.  The current
assertion states that it's incorrect to have a lazy value where the type
does not match the enclosing_type (I don't really know why), but it
clearly happens here.  So I'd like to determine who's right, the
assertion or the code that produces such a value.  For reference, the
assertion was added here:

  https://pi.simark.ca/gdb-patches/m3hbmbviko.fsf@fleche.redhat.com/#t

I was wondering at which point exactly we end up with such a value.
It's here that we assign a new type (the array type) to the value, while
the value is lazy, when handling the value cast:

    #0  deprecated_set_value_type (value=0x61100020a700, type=0x621000506890) at /home/smarchi/src/binutils-gdb/gdb/value.c:1105
    #1  0x0000564249d99092 in value_cast (type=0x621000506730, arg2=0x61100020a700) at /home/smarchi/src/binutils-gdb/gdb/valops.c:496
    #2  0x000056424838294c in expr::var_value_operation::evaluate_for_cast (this=0x6030002a6220, to_type=0x621000506730, exp=0x60300011cbc0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/eval.c:2885
    #3  0x0000564247412e1e in expr::unop_cast_type_operation::evaluate (this=0x6030002a6250, expect_type=0x0, exp=0x60300011cbc0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/expop.h:2036
    #4  0x000056424836092c in expression::evaluate (this=0x60300011cbc0, expect_type=0x0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/eval.c:101
    #5  0x0000564248360a9d in evaluate_expression (exp=0x60300011cbc0, expect_type=0x0) at /home/smarchi/src/binutils-gdb/gdb/eval.c:115
    #6  0x0000564248e56c95 in process_print_command_args (args=0x60200006fb52 "(int []) s1", print_opts=0x7ffe0fb62200, voidprint=true) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1307
    #7  0x0000564248e56e79 in print_command_1 (args=0x60200006fb52 "(int []) s1", voidprint=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1320
    #8  0x0000564248e57d15 in print_command (exp=0x60200006fb52 "(int []) s1", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1453

Meanwhile, the enclosing_type remains the original struct type.

So my question is: when doing the value cast, do we also want to
overwrite the value's enclosing type, in addition to the value's type?
Is it useful to retain the original enclosing type?  In other words, we
could do this change to also set the enclosing type when casting:


diff --git a/gdb/valops.c b/gdb/valops.c
index 27e84d9f6b3..cb3fb49e50d 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -493,10 +493,10 @@ value_cast (struct type *type, struct value *arg2)
                                                 TYPE_TARGET_TYPE (range_type),
                                                 low_bound,
                                                 new_length + low_bound - 1);
-         deprecated_set_value_type (arg2,
-                                    create_array_type (NULL,
-                                                       element_type,
-                                                       range_type));
+         struct type *new_type
+           = create_array_type (NULL, element_type, range_type);
+         deprecated_set_value_type (arg2, new_type);
+         set_value_enclosing_type (arg2, new_type);
          return arg2;
        }
     }

I don't know if that change causes other things to fail, and I really
don't know what the answer is.

One case I wonder about is when the size of the original object is not a
multiple of the array's element size.  For instance, imagine s1 is 9
bytes long and you do:

  (gdb) print (int []) s1

where an int is 4 bytes.  The pieces of s1 will still describe 9 bytes,
but the type and enclosing type will be 8 bytes long.  That might be a
problem down the road, and a reason we want to keep the original
enclosing type whose length matches the described content length.  Some
additional tests for that would be nice.

> ---
>  gdb/dwarf2/expr.c                       |  6 ++----
>  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index 99862583336..6330a5787fc 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool check_optimized)
>      }
>    else
>      {
> -      if (value_type (v) != value_enclosing_type (v))
> -	internal_error (__FILE__, __LINE__,
> -			_("Should not be able to create a lazy value with "
> -			  "an enclosing type"));
> +      gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset (v))
> +		  <= TYPE_LENGTH (value_enclosing_type (v)));

This looks like an assertion that should hold for all values, all the
time.  When a value has an enclosing type different than its type, the
type should be strictly a subset of the enclosing type.  So, it seems a
bit odd to check this here specifically.  It would maybe make more sense
to check it when modifying the type or enclosing_type of a value.  In
any case, that's separate from the current patch.  If we find out that
it is correct to remove the existing internal_error call, then I would
just simply remove it (replace it with nothing).

Simon

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

[Rohr, Stephan]

Hi Simon,

thanks for the feedback and sorry for the late reply.  Please see my answers below.

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Friday, August 19, 2022 6:40 PM
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Cc: tom@tromey.com
> Subject: Re: [PATCH 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted
> to different type.
> 
> On 4/22/22 10:44, 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.
> 
> Out of curiosity, where do you see these scenarios described?  I could not
> find where value.c talks about the case where the user casts the variable.
> 
This scenario is not described in value.c.  The issue came up in our test suite
where the assertion caused GDB to crash when casting a pieced value, e.g.,

(gdb) info address a
Symbol "a" is multi-location:
  Range 0xfffd5990-0xfffd5e50: a variable in $r68 [256-bit piece, offset 0 bits],
and a variable in $r69 [256-bit piece, offset 0 bits], ...
(gdb) pt a
type = double [16][4]
(gdb) p (double[])a
.../gdb/dwarf2/loc.c:1771: internal-error: Should not be able to create a lazy value with an enclosing type
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

> >
> > 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, it should be
> > sufficient to check if the type's length (potentially shifted by
> > 'embedded_offset') does not exceed the enclosing type's length which
> > was used for memory allocation.
> 
> I'm trying to understand a bit better what happens here.  The current
> assertion states that it's incorrect to have a lazy value where the type does
> not match the enclosing_type (I don't really know why), but it clearly
> happens here.  So I'd like to determine who's right, the assertion or the code
> that produces such a value.  For reference, the assertion was added here:
> 
>   https://pi.simark.ca/gdb-patches/m3hbmbviko.fsf@fleche.redhat.com/#t
> 

I contacted Tom when I first encountered this issue.  Based on his feedback,
it seems fine to remove the assertion if the results work correctly.  With your
feedback and the test cases I added, I would agree that we can remove the
original assertion, though I also don't know why it was originally added.

> I was wondering at which point exactly we end up with such a value.
> It's here that we assign a new type (the array type) to the value, while the
> value is lazy, when handling the value cast:
> 
>     #0  deprecated_set_value_type (value=0x61100020a700,
> type=0x621000506890) at /home/smarchi/src/binutils-gdb/gdb/value.c:1105
>     #1  0x0000564249d99092 in value_cast (type=0x621000506730,
> arg2=0x61100020a700) at /home/smarchi/src/binutils-gdb/gdb/valops.c:496
>     #2  0x000056424838294c in expr::var_value_operation::evaluate_for_cast
> (this=0x6030002a6220, to_type=0x621000506730, exp=0x60300011cbc0,
> noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/eval.c:2885
>     #3  0x0000564247412e1e in expr::unop_cast_type_operation::evaluate
> (this=0x6030002a6250, expect_type=0x0, exp=0x60300011cbc0,
> noside=EVAL_NORMAL) at /home/smarchi/src/binutils-
> gdb/gdb/expop.h:2036
>     #4  0x000056424836092c in expression::evaluate (this=0x60300011cbc0,
> expect_type=0x0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-
> gdb/gdb/eval.c:101
>     #5  0x0000564248360a9d in evaluate_expression (exp=0x60300011cbc0,
> expect_type=0x0) at /home/smarchi/src/binutils-gdb/gdb/eval.c:115
>     #6  0x0000564248e56c95 in process_print_command_args
> (args=0x60200006fb52 "(int []) s1", print_opts=0x7ffe0fb62200,
> voidprint=true) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1307
>     #7  0x0000564248e56e79 in print_command_1 (args=0x60200006fb52 "(int
> []) s1", voidprint=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1320
>     #8  0x0000564248e57d15 in print_command (exp=0x60200006fb52 "(int [])
> s1", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1453
> 
> Meanwhile, the enclosing_type remains the original struct type.
> 
> So my question is: when doing the value cast, do we also want to overwrite
> the value's enclosing type, in addition to the value's type?
> Is it useful to retain the original enclosing type?  In other words, we could do
> this change to also set the enclosing type when casting:
> 
> 
> diff --git a/gdb/valops.c b/gdb/valops.c index 27e84d9f6b3..cb3fb49e50d
> 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -493,10 +493,10 @@ value_cast (struct type *type, struct value *arg2)
>                                                  TYPE_TARGET_TYPE (range_type),
>                                                  low_bound,
>                                                  new_length + low_bound - 1);
> -         deprecated_set_value_type (arg2,
> -                                    create_array_type (NULL,
> -                                                       element_type,
> -                                                       range_type));
> +         struct type *new_type
> +           = create_array_type (NULL, element_type, range_type);
> +         deprecated_set_value_type (arg2, new_type);
> +         set_value_enclosing_type (arg2, new_type);
>           return arg2;
>         }
>      }
> 
> I don't know if that change causes other things to fail, and I really don't know
> what the answer is.

This would surely fix the original issue I encountered.  However, I also see the risk
of unexpected side effects.

> One case I wonder about is when the size of the original object is not a
> multiple of the array's element size.  For instance, imagine s1 is 9 bytes long
> and you do:
> 
>   (gdb) print (int []) s1
> 
> where an int is 4 bytes.  The pieces of s1 will still describe 9 bytes, but the
> type and enclosing type will be 8 bytes long.  That might be a problem down
> the road, and a reason we want to keep the original enclosing type whose
> length matches the described content length.  Some additional tests for that
> would be nice.
> 

I extended the shortpiece.exp testcase by adding another variable s3 that is 10
bytes long.  GDB emits a warning and prints the maximum number of integers:

p (int []) s3
warning: array element type size does not divide object size in cast
$5 = {0, 1}

This is expected and correct behavior from my point of view.

> > ---
> >  gdb/dwarf2/expr.c                       |  6 ++----
> >  gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c index
> > 99862583336..6330a5787fc 100644
> > --- a/gdb/dwarf2/expr.c
> > +++ b/gdb/dwarf2/expr.c
> > @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool
> check_optimized)
> >      }
> >    else
> >      {
> > -      if (value_type (v) != value_enclosing_type (v))
> > -	internal_error (__FILE__, __LINE__,
> > -			_("Should not be able to create a lazy value with "
> > -			  "an enclosing type"));
> > +      gdb_assert ((TYPE_LENGTH (value_type (v)) +
> value_embedded_offset (v))
> > +		  <= TYPE_LENGTH (value_enclosing_type (v)));
> 
> This looks like an assertion that should hold for all values, all the time.  When
> a value has an enclosing type different than its type, the type should be
> strictly a subset of the enclosing type.  So, it seems a bit odd to check this
> here specifically.  It would maybe make more sense to check it when
> modifying the type or enclosing_type of a value.  In any case, that's separate
> from the current patch.  If we find out that it is correct to remove the existing
> internal_error call, then I would just simply remove it (replace it with
> nothing).

I agree here.  I will submit a new version of the patch with the extended testcase
and the removed assertion if we conclude that it's safe to remove.

> Simon

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

[Tom Tromey]

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

Simon> I'm trying to understand a bit better what happens here.  The current
Simon> assertion states that it's incorrect to have a lazy value where the type
Simon> does not match the enclosing_type (I don't really know why)

I'm not 100% sure either, but I imagine the logic is that to have a
different enclosing type, gdb must have examined the value, dug through
its vtable, and found the true runtime type.  But if this happened, then
the value can't be lazy any more.

Simon> So my question is: when doing the value cast, do we also want to
Simon> overwrite the value's enclosing type, in addition to the value's type?
Simon> Is it useful to retain the original enclosing type?

I think leaving it alone is probably wrong and could possible give other
weird results.

Tom

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

[Tom Tromey]

> I agree here.  I will submit a new version of the patch with the extended testcase
> and the removed assertion if we conclude that it's safe to remove.

BTW there is a bug in bugzilla about this:
https://sourceware.org/bugzilla/show_bug.cgi?id=28605

It would be good to mention this in the commit message.

Tom