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

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

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