Re: [PATCH 2/6] gdb: use TARGET_CHAR_BIT more in dwarf2/expr.c

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> A later commit in this series touches some code in dwarf2/expr.c that
> Andrew> used "8 * some_byte_count" to convert to a bit count.  It seemed
> Andrew> natural to change this to "TARGET_CHAR_BIT * some_byte_count", but
> Andrew> when I started looking I realised there's a lot of places in
> Andrew> dwarf2/expr.c that use "8" instead of "TARGET_CHAR_BIT".
>
> Andrew> So I split out this commit to change all of the uses of "8" to
> Andrew> "TARGET_CHAR_BIT".  I believe everywhere I've changed really is doing
> Andrew> conversions between bits and bytes, so this change is appropriate.
>
> FWIW I think there's a lot more code around using '8' (not your problem
> obviously); but more importantly that TARGET_CHAR_BIT can't really be
> the correct approach to this problem.
>
> For TARGET_CHAR_BIT to work, I think we'd instead have to change all
> these spots to reference some gdbarch (i.e. in addition to its other
> problems, "TARGET" is a misnomer).
>
> Also, I think some places using TARGET_CHAR_BIT incorrectly mix host and
> target side data.
>
> But, luckily, I also tend to think it's a problem that won't ever need
> solving.
>
> Andrew>  		if (!check_optimized)
> Andrew>  		  copy_bitwise (v_contents, offset,
> Andrew> -				buffer.data (), bits_to_skip % 8,
> Andrew> +				buffer.data (), bits_to_skip % TARGET_CHAR_BIT,
> Andrew>  				this_size_bits, bits_big_endian);
>
> In this situation it seems to me that gdb is manipulating host-side data.

Aren't the value contents pulled from the inferior though?  And
bits_to_skip and this_size_bits are all from DWARF properties that
describe the inferior's data, so I don't understand why TARGET_CHAR_BIT
wouldn't be correct?  I got really confused trying to reason this all
through.

Maybe the 8-bit byte assumption is just baked into GDB too much?  In
this case the BUFFER is filled by get_frame_register_bytes.  Is this
host bytes?  Target bytes?  Is the '%' here trying to adjust the
inferior bits_to_skip so we can index into a host byte buffer?  Or
express a target bits_to_skip and copy_bitwise will figure out any
conversion?  Of course copy_bitwise actually just hard-codes '8'
everywhere, so the question is mostly pointless.

I eventually just decided to drop this patch from the series.  There
were just 3 lines in the final patch where I wanted to change from '8'
to 'TARGET_CHAR_BIT', and in that case I think it's much clearer that
TARGET_CHAR_BIT is the correct choice.

This patch started as me factoring out those 3 lines, and then thinking:
I wonder if there are other places that I could update.

But it's now just a distraction from getting the actual fixes into GDB,
so I'd rather just get those merged, and return to this another day.

Thanks for looking at this series.  I've now pushed all the other
patches.

Thanks,
Andrew