Re: [PATCH, ARM] Fix out-of-range immediate assembly errors on 64-bit hosts

[Nick Clifton <nickc at redhat dot com>]

Hi Julian,

> > Sorry - can you explain that last part again please ?  I get how 
> > INT_MIN and INT_MAX are being used to indicate "any integer value is 
> > acceptable" to parse_immediate(), but where does this 0xffff0000 value 
> > come from and why would it cause problems for parse_immediate() ?
>
> 0xffff0000 is parsed as a positive integer when 
> sizeof(exp.X_add_number)==8, and a negative integer when it equals 4. 

And you are saying that "0xffff0000 > INT_MAX" for a 64-bit host ? 
(This was the bit that I had not caught, sorry).

> I don't know why it didn't occur to me to use LONG_MIN, LONG_MAX at the 
> caller instead, but that seems a bit wrong too somehow.

Yes, although it might be a simpler change.

> I think it'd 
> probably work, but maybe it'd be better to add a 
> bounds-checked/non-bounds-checked argument to parse_immediate() or write 
> a non-bounds-checking version of the function instead?

I think that this would be the best solution.  It makes the meaning of 
the arguments passed to parse_immediate more obvious.


> > I am a little bit confused here.  How is ((x >> 16) >> 16) different 
> > from (x >> 32) ?  If not, then why express it that way ?
>
> If sizeof(exp.X_add_number)==4 (i.e. on 32-bit hosts), then I think that 
> writing (x >> 32) is invalid C (which isn't very nice, even if that code 
> would never be executed).

Ah, yes, I had not considered that.

> Is there a less weird way of writing the same thing?

Not really.  You use a macro I guess.  eg:

  /* Do not use ">> 32" as this will trigger a warning on hosts
     where sizeof (typeof (a)) == 4.  */
  #define get_top_32_bits(a)  (((a) >> 16) >> 16) & 0xffffffff)

Or just have the comment in the code.  (Hmm, do we need to worry about 
compiling on 16-bit hosts where ">> 16" would trigger a warning ?)

Cheers
  Nick