Re: i386-dis.c UB shift and other tidies

[Jan Beulich <jbeulich@suse.com>]

On 26.04.2023 06:39, Alan Modra via Binutils wrote:
> 1) i386-dis.c:12055:11: runtime error: left shift of negative value -1
> Bit twiddling is best done unsigned, due to UB on overflow of signed
> expressions.  Fix this by using bfd_vma rather than bfd_signed_vma
> everywhere in i386-dis.c except print_displacement.
> 
> 2) Return get32s and get16 value in a bfd_vma, reducing the need for
> temp variables.

While I'm okay with most other changes, I find it pretty odd for any of
...

> 3) Introduce get16s and get8s functions to simplify the code.

... the get<N>s() functions to return an unsigned quantity. This then
leads to ugly casting in at least OP_E_memory() (once explicit, once
implicit). If they want to do some of the calculations (shifts in
particular, as you say) using unsigned intermediate types, that's of
course fine.

As a minor remark - because of you switching some of the get16() to
get16s(), get16() doesn't need a forward declaration anymore (just
like get32() and get64() don't have such [anymore]). (Ultimately I'd
like to get rid of as many forward declarations of static functions
as possible, because this always requires touching yet one more place
when changing their signatures.)

> 4) With some optimisation options gcc-13 legitimately complains about
> a fall-through in OP_I.  Fix that.  OP_I also doesn't need to use
> "mask" which was wrong for w_mode anyway.

While making the earlier recent change I was puzzled by that, too,
but I deliberately left it untouched. I'm afraid we don't really have
any test for it, and it looked to me as if that masking was
intentionally done that (odd) way. (This isn't an objection to the
change, but I wouldn't be surprised if something subtly broke, which
we'd then need to take care of later on.)

> 5) Masking with & 0xffffffff is better than casting to unsigned.  We
> don't know for sure that unsigned int is 32-bit.
> 
> 6) We also don't know that unsigned char is 8 bits.  Mask codep
> accesses everywhere.  I don't expect binutils will work on anything
> other than an 8-bit char host, but if we are masking codep accesses in
> some places we might as well be consistent.  (Better would be to use
> stdint.h types more in binutils.)

Would there be anything wrong with switching codep to uint8_t * right
away, in place of all the masking by 0xff that you add? When also done
for its *_codep siblings, at the first glance this looks to not require
much further touching (and hence presumably overall less code churn;
existing masking that then clearly isn't necessary anymore could of
course be purged right away, but this could also be left for later).

Jan