Re: [PATCH v2] [gdb/exp] Fix printing of out of bounds struct members

[Simon Marchi <simark@simark.ca>]

On 2024-01-24 11:10, Tom de Vries wrote:
> diff --git a/gdb/value.c b/gdb/value.c
> index 4ec9babcce8..373b3d929da 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1229,6 +1229,9 @@ value::contents_copy_raw (struct value *dst, LONGEST dst_offset,
>    gdb_assert (!dst->bits_any_optimized_out (TARGET_CHAR_BIT * dst_offset,
>  					    TARGET_CHAR_BIT * length));
>  
> +  if ((src_offset + copy_length) * unit_size > enclosing_type ()-> length ())
> +    error (_("access outside bounds of object"));

I stumbled on this randomly because I'm looking at conflict resolution
downstream involving this (due to an unrelated change downstream).  I'm
not convinced that a low-level function like this is the right place to
throw an error.  I think that if we get here trying to do an out of
bounds copy, it's a symptom of something that went wrong earlier, and
there's usually a better way to address it.

In this case, my understanding is that:

 - the expression for DW_AT_byte_size, given the uninitialized state of
   the variable, yields 8 - as if `A` was true.
 - the description of the variant, given the uninitialized state of the
   variable, tells us that fields C and D are active - as if `B` was
   false.  When we sum the size of all the active fields of the type, it
   gives us 12.

So, it seems to me like the debug information contradicts itself in this
case.  It can't tell us that the type is 8 bytes long but then tell us
that 12 bytes worth of fields are active.  Or, it should be able to
tells us what is the PC range in which the variable can be considered
initialized (I think this is a longstanding issue with DWARF).

I think that a first step that could be taken by the compiler today to
make the debug info more robust would be to not use a default variant
case.  When seeing an invalid discriminant value, GDB would avoid
printing the variant fields altogether.

As for how to handle the current situation in GDB today, I would perhaps
suggest to at least move the bounds check / error call up one level, to
`value::primitive_field`.

Another option could be to ignore the DW_AT_byte_size for the size of
resolved types (rely on the sum of the sizes of the active fields).  But
I don't know the implications of that, the code that overrides the
resolved type's size here [1] perhaps exists for a good reason.

Simon

[1] https://gitlab.com/gnutools/binutils-gdb/-/blob/b47cef7ca8a2fa55acdab367c87c1ea076aec4b2/gdb/gdbtypes.c#L2849-2853