Re: RFC: fix bug in pieced value with offset

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 14 May 2010 19:29:33 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> Thinking now if the BFD_ENDIAN_BIG patch by Ulrich Weigand
> Jan> 	[rfc] Handle DWARF-2 value pieces residing in *parts* of a register
> Jan> 	http://sourceware.org/ml/gdb-patches/2009-12/msg00305.html
> Jan> should not have been applied also for DWARF_VALUE_STACK; but this
> Jan> is outside of the scope of this patch.
> 
> I must be missing something... I don't see any change to
> DWARF_VALUE_STACK there.

I am sorry, s/should not have/should have/.  Does it make sense now?


> Jan> I believe it should be instead:
> Jan> # +	      reg_offset = (register_size (arch, gdb_regnum)
> Jan> # +			    - this_size);
> 
> Jan> As we should ignore source_offset bytes from the start of register.
> Jan> register_size = 8
> Jan> p-> size = 4
> Jan> bytes_to_skip = for example 1
> Jan> =>
> Jan> source_offset = 1
> Jan> this_size = 3
> 
> Jan> From the register occupying bytes <0..7> we thus want to read-in
> Jan> bytes <5..7>.
> 
> My thinking was to consider the resulting contents as a sequence of
> bytes.  In this case the register would be laid out from high byte to
> low byte.

yes.

> The existing 'size' offsetting strips off high bytes (because
> it is conceptually value-based); but then for 'source_offset' we want to
> advance through the byte representation -- so, also skipping high bytes.

Following my example of parameters written in my mail above.
Let's have in big-ending 64bit register value 0xdeadf00dbaadbeef.
It will be laid out inside get_frame_register_bytes as:

offset: 0     1     2     3     4     5     6     7
value:  0xde, 0xad, 0xf0, 0x0d, 0xba, 0xad, 0xbe, 0xef

This piece P is for `DW_OP_piece: 4'.

value_offset is 1 (or larger if preceding pieces were involved).

Therefore I believe we should read in 0xad, 0xbe, 0xef.

Your patch reads in 0xba, 0xad, 0xbe.


> I can't tell if this makes sense or not.

I am not sure if your sentence was agreeing with my suggested change or not.


> >> 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;
> 
> Jan> Generally I would prefer more sanity checks there instead of quiet data
> Jan> cutting.
> 
> In this particular case, I think this is just what DWARF specifies.

I find this case (n > c->addr_size) as not described => undefined by DWARF.


> I think it makes sense to compute a value on the stack and then just
> select some bits from it.

I agree with your sentence.  I do not agree your sentence describes your code.
The comparison present in code is exactly the opposite one.  Your sentence
describes "n < c->addr_size".

DWARF DW_OP_piece must be arch aware.  arch (specifically cu_header.addr_size)
specifies c->addr_size.  DW_OP_piece parameter N must be aware of c->addr_size.
DWARF4 2.5.1 says:
	Each element of the stack is the size of an address on the target
	machine.

Therefore I find DW_OP_piece trying to use more bytes from DWARF_VALUE_STACK
(DW_OP_piece must be aware it is referencing a DWARF_VALUE_STACK value) than
what is size of the DWARF_VALUE_STACK object as an error in input DWARF.


> Maybe for DWARF_VALUE_LITERAL it would make sense to issue a complaint
> if the piece is smaller than the literal.  That would be strange
> compiler output.

I did not think about this case.  I agree.


Thanks,
Jan