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

[Simon Marchi <simark@simark.ca>]

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