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

[Tom Tromey <tom@tromey.com>]

>>>>> "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.

So here, using '8' (really HOST_CHAR_BIT) seems more appropriate, since
otherwise the wrong number of bits will be manipulated.

I didn't go through all the spots to find which are which.  And TBH
unless someone is really porting to such an architecture, it seems a
little pointless, in the sense that there isn't a good way to test this
anyway -- and even if we came up with one through some heroics, would we
really want to bother?

Andrew> -  bit_offset += 8 * value->offset ();
Andrew> +  bit_offset += TARGET_CHAR_BIT * value->offset ();

Other spots like this do make sense, because the bit offset is a
function of the size of a target byte.

Anyway, despite all this I have no objection to this patch and I even
approve it, with my reasons being that (1) TARGET_CHAR_BIT at least
indicates places that maybe need to be examined should anyone ever do
this work (though searching for "8" does the same); (2) anyway there are
probably already many incorrect uses in the tree; and (3) using a name
is better than "8".

Approved-By: Tom Tromey <tom@tromey.com>

Tom