Re: RFC: fix bug in pieced value with offset

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 13 May 2010 19:04:55 +0200, Tom Tromey wrote:
> --- dwarf2loc.c	13 May 2010 15:44:35 -0000	1.77
> +++ dwarf2loc.c	13 May 2010 17:01:01 -0000
# @@ -277,17 +306,18 @@ read_pieced_value (struct value *v)
# case DWARF_VALUE_REGISTER:
>  	    struct gdbarch *arch = get_frame_arch (frame);
>  	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch,
>  							   p->v.expr.value);
> -	    int reg_offset = 0;
> +	    int reg_offset = source_offset;
>  
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& p->size < register_size (arch, gdb_regnum))
> +		&& this_size + reg_offset <= register_size (arch, gdb_regnum))
>  	      /* Big-endian, and we want less than full size.  */
> -	      reg_offset = register_size (arch, gdb_regnum) - p->size;
> +	      reg_offset = (register_size (arch, gdb_regnum)
> +			    - this_size - reg_offset);

Thinking now if the BFD_ENDIAN_BIG patch by Ulrich Weigand
	[rfc] Handle DWARF-2 value pieces residing in *parts* of a register
	http://sourceware.org/ml/gdb-patches/2009-12/msg00305.html
should not have been applied also for DWARF_VALUE_STACK; but this is outside
of the scope of this patch.

I think here is missing some size/validity warning()/error() as discussed for
DWARF_VALUE_STACK below.  In such case the whole condition could be removed:
> -             && p->size < register_size (arch, gdb_regnum))
> +             && this_size + reg_offset <= register_size (arch, gdb_regnum))

I believe it should be instead:
# +	      reg_offset = (register_size (arch, gdb_regnum)
# +			    - this_size);
                                       ^^^
As we should ignore source_offset bytes from the start of register.
register_size = 8
p->size = 4
bytes_to_skip = for example 1
=>
source_offset = 1
this_size = 3

>From the register occupying bytes <0..7> we thus want to read-in bytes <5..7>.

(Currently we assume we always want to read up to infinity as you noted in the
followup mail:
	http://sourceware.org/ml/gdb-patches/2010-05/msg00282.html
In reality we may want to read less than up to <..7>.)


>  	case DWARF_VALUE_STACK:
>  	  {
>  	    struct gdbarch *gdbarch = get_type_arch (value_type (v));
> -	    size_t n = p->size;
> +	    size_t n = this_size;
>  	    if (n > c->addr_size)
>  	      n = c->addr_size;

Generally I would prefer more sanity checks there instead of quiet data
cutting.  Moreover for example get_frame_register_bytes() currently performs
non-fatal warning:
    {
      warning (_("Bad debug information detected: "
                 "Attempt to read %d bytes from registers."), len);
      return 0;
    }
while IMO in these cases it could error().

There is also missing `- source_offset':
#  	    if (n > c->addr_size - source_offset)
#  	      n = c->addr_size - source_offset;


> -	    store_unsigned_integer (contents + offset, n,
> -				    gdbarch_byte_order (gdbarch),
> -				    p->v.expr.value);
> +	    if (source_offset == 0)
> +	      store_unsigned_integer (contents + dest_offset, n,
> +				      gdbarch_byte_order (gdbarch),
> +				      p->v.expr.value);
> +	    else
> +	      {
> +		gdb_byte bytes[sizeof (ULONGEST)];

Missing an empty line for declarations separation.

> +		store_unsigned_integer (bytes, n,

Instead of `n' there should be `n + source_offset'.

> +					gdbarch_byte_order (gdbarch),
> +					p->v.expr.value);
> +		memcpy (contents + dest_offset, bytes + source_offset, n);
> +	      }
>  	  }
>  	  break;
>  
>  	case DWARF_VALUE_LITERAL:
>  	  {
> -	    size_t n = p->size;
> -	    if (n > p->v.literal.length)
> -	      n = p->v.literal.length;
> -	    memcpy (contents + offset, p->v.literal.data, n);
> +	    if (this_size > p->v.literal.length)
> +	      this_size = p->v.literal.length;

Again missing `- source_offset' and it should be IMO more some error():

# +	    if (this_size > p->v.literal.length - source_offset)
# +	      this_size = p->v.literal.length - source_offset;


# @@ -347,27 +389,51 @@ write_pieced_value (struct value *to, struct value *from)
[...]
>        switch (p->location)
>  	{
>  	case DWARF_VALUE_REGISTER:
>  	  {
>  	    struct gdbarch *arch = get_frame_arch (frame);
>  	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.expr.value);
> -	    int reg_offset = 0;
> +	    int reg_offset = dest_offset;
>  

as in read_pieced_value: Missing offset/size validation.

>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG

> -		&& p->size < register_size (arch, gdb_regnum))
> +		&& this_size + reg_offset <= register_size (arch, gdb_regnum))

as in read_pieced_value: The conditional could be probably removed.

>  	      /* Big-endian, and we want less than full size.  */
> -	      reg_offset = register_size (arch, gdb_regnum) - p->size;
> +	      reg_offset = (register_size (arch, gdb_regnum) - this_size
> +			    - reg_offset);

as in read_pieced_value: Excessive `- reg_offset'.


> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ testsuite/gdb.dwarf2/pieces.S	13 May 2010 17:01:03 -0000
[...]
> +.globl _start
> +	.type	_start, @function
> +_start:

Together with:
> +       [list {additional_flags=-nostdlib}]] != "" } {

Why weren't just simple "main" and standard compilation used?  It works for
me.


> Index: testsuite/gdb.dwarf2/pieces.c
> ===================================================================
> RCS file: testsuite/gdb.dwarf2/pieces.c
> diff -N testsuite/gdb.dwarf2/pieces.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ testsuite/gdb.dwarf2/pieces.c	13 May 2010 17:01:03 -0000
> @@ -0,0 +1,80 @@
> +/* The original program corresponding to pieces.c.
> +   This originally came from https://bugzilla.redhat.com/show_bug.cgi?id=589467
> +   Note that it is not ever compiled, pieces.S is used instead.  */

Isn't missing the ubiquitous FSF copyleft header?


> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ testsuite/gdb.dwarf2/pieces.exp	13 May 2010 17:01:03 -0000
> @@ -0,0 +1,60 @@
[...]
> +set testfile "pieces"
> +set srcfile ${testfile}.S
> +set binfile ${objdir}/${subdir}/${testfile}.x
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
> +       [list {additional_flags=-nostdlib}]] != "" } {
> +    return -1
> +}

If -nostdlib would get removed by using "main" then we could
prepare_for_testing.


> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

clean_restart ${testfile}.x


> +# Function f1 tests a particular gdb bug involving DW_OP_piece.
> +proc pieces_test_f1 {} {
> +    gdb_test "break pieces.c:22" "Breakpoint 2.*" \
> +	"set first breakpoint for pieces"
> +    gdb_continue_to_breakpoint "first continue to breakpoint for pieces"

I would use also the second parameter to verify the stop point but it is a bit
controversial whether to use it:
#      gdb_continue_to_breakpoint "first continue to breakpoint for pieces" ".*pieces.c:22\r\n.*"


Thanks,
Jan