Re: [PATCH v3 06/28] Add read method to location description classes

[Zoran Zaric <zoran.zaric@amd.com>]

On 11/3/21 7:03 PM, Simon Marchi wrote:
> [CAUTION: External Email]
> 
>> The problem that I have with this idea is that DWARF doesn't speak in
>> terms of addressable memory units, but in terms of byte representation of an architecture.
>>
>> The addressable memory units only describe what an underlying memory read write interface need to consider, but from a location description point of view we need a byte/bit correlation that describes all location description kinds uniformly and the addressable memory units is not it.
>>
>> For example, how would this work in the case of registers? In most architectures they are only addressable as a whole.
>>
>> Another problem here is that the m_offset is not equivalent to offset
>> or embedded offset from struct value. This is especially true for memory location descriptions where the m_offset member is an offset from the beginning of the memory location as a whole aka the address itself.
>>
>> Previously the only way to add an offset to a memory location is to implicitly convert it to the value on the expression stack, add an offset to it and then convert it back to an address. This means that if the expression yielded unaddressable address, the read would just fail and the gdb would not try to compensate by reading more then copy with an offset.
>>
>> Considering all of this, the only correct thing to do here is either use hard coded value of 8 (like was done before my changes) or use the TARGET_BYTE_CHAR instead until there is a need for a proper byte/bit correlation target hook.
>>
>> I can also see that we are using gdbarch_addressable_memory_unit_size
>> call for some struct value API (for example value_contents_copy) for offset and embedded offset regardless of the location description kind.
>>
>> This is obviously wrong, but it was not detected from the DWARF side because the only way how one can add an offset to a register location is through the lval_computed interface which goes around these issues.
>>
>> Because now this is not the case, I can solve this issue by making sure that when a dwarf_entry class is converted to a struct value object, the struct value offset member (and the corresponding bit offset member) are still talking about offset in terms of addressable units for all locations description kinds.
> 
> I spoke with Zoran offline about this.  In the end, the conclusion is
> that there are too many unknowns on how things are expected to work with
> non-8-bit-bytes targets.  On one hand, the DWARF standard makes it
> sound (section 1.3.2 of DWARF 5) like a "byte" can be other sizes than 8
> bits:
> 
>      DWARF can be used with a wide range of processor architectures, whether byte
>      or word oriented, linear or segmented, with any word or byte size.
> 
> So when you apply an offset measured in bytes in a location, that would
> be using the target processor byte size.  And that would apply to all
> locations, registers, memory and others (especially since you want to
> treat all locations kinds the same).
> 
> But as you have mentioned, all areas of GDB are not consistent regarding
> this.  Some code paths today use an hard-coded 8 when applying offsets.
> I think this is more related to the fact that when we upstreamed these
> bits (in my previous life), we changed whatever code paths our GDB was
> taking, not all.
> 
> Since there isn't even one architecture upstream using non-8-bit-bytes,
> you can't even take a peek to see how it works.  So I would say, do
> whatever is the easiest for you (except using TARGET_CHAR_BIT :)).  If
> that's assuming 8-bit offsets everywhere, go for it.  If it's using
> gdbarch_addressable_memory_unit_size-sized offsets for all location
> kinds, go for it.  This means the behavior of GDB for non-8-bit-bytes
> target can change, but IMO that's ok.  The current code is likely not
> entirely correct as is anyway.  And if somebody downstream is still
> using GDB for such a target, they'll have to contribute fixes if they
> want to.  We can't guess what's right.  And if nobody is using such a
> target downstream, then it doesn't change anything.
> 
> Simon
> 

I agree with this. Also, the reason why I use the HOST_CHAR_SIZE is 
because Pedro Alves suggested for me to use it instead of the hard coded 
number 8 because apparently, that constant is always guaranteed to have 
a value 8.

Thanks for all the reviews guys. I've just uploaded a new series with 
all the fixes.

Zoran