i386-dis.c UB shift and other tidies

[Alan Modra]

On Wed, Apr 26, 2023 at 10:09:13AM +0200, Jan Beulich wrote:
> 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.

Change it back if you find it ugly, but I've found it better to
default to using unsigned types of the largest width you are typically
going to manipluate.

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

Which is ugly too, and we often forget to avoid UB by casts or
assigning to unsigned types.

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

I missed that.  Maybe you should move things around in the file?

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

I checked carefully.  It really is unneeded when you consider the
range of values returned by the get* functions.  get16 is limited to
[0,0xffff], so masking with 0xfffff does nothing.

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

I think switching would be a good idea.  I've changed bfd_byte (not
yet committed) and that seems good.

-- 
Alan Modra
Australia Development Lab, IBM