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

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

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

Hi,

This patch fixes some immediate-out-of-range errors for VBIC, VORR etc. 
on 64-bit hosts. On such hosts, the X_add_number field of expressions 
will be 64 bits wide. INT_MIN and INT_MAX are used to signify that any 
immediate may be accepted for parse_immediate: unfortunately on a 64-bit 
host, 0xff000000 for instance is then interpreted as a positive integer 
outside that range. I've just made passing INT_MIN/INT_MAX disable the 
check instead, though I'm not very fond of that solution.

Tested with cross to arm-none-eabi from x86_64-unknown-linux-gnu.

OK to apply?

Cheers,

Julian

ChangeLog (gas):

     * config/tc-arm.c (parse_immediate): Handle 64-bit X_add_number
     case.
     (parse_big_immediate): Likewise.

[-- Attachment #2: neon-asm-64bit-host-1 --]
[-- Type: text/plain, Size: 1875 bytes --]

Index: config/tc-arm.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-arm.c,v
retrieving revision 1.274
diff -c -p -r1.274 tc-arm.c
*** config/tc-arm.c	7 Jun 2006 14:32:28 -0000	1.274
--- config/tc-arm.c	9 Jun 2006 16:41:36 -0000
*************** parse_immediate (char **str, int *val, i
*** 3887,3893 ****
        return FAIL;
      }
  
!   if (exp.X_add_number < min || exp.X_add_number > max)
      {
        inst.error = _("immediate value out of range");
        return FAIL;
--- 3887,3894 ----
        return FAIL;
      }
  
!   if ((min != INT_MIN || max != INT_MAX)
!       && (exp.X_add_number < min || exp.X_add_number > max))
      {
        inst.error = _("immediate value out of range");
        return FAIL;
*************** parse_big_immediate (char **str, int i)
*** 3910,3916 ****
    my_get_expression (&exp, &ptr, GE_OPT_PREFIX_BIG);
  
    if (exp.X_op == O_constant)
!     inst.operands[i].imm = exp.X_add_number;
    else if (exp.X_op == O_big
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number > 32
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number <= 64)
--- 3911,3928 ----
    my_get_expression (&exp, &ptr, GE_OPT_PREFIX_BIG);
  
    if (exp.X_op == O_constant)
!     {
!       inst.operands[i].imm = exp.X_add_number & 0xffffffff;
!       /* If we're on a 64-bit host, then a 64-bit number can be returned using
! 	 O_constant.  We have to be careful not to break compilation for
! 	 32-bit X_add_number, though.  */
!       if ((exp.X_add_number & ~0xffffffffl) != 0)
! 	{
!           inst.operands[i].reg = ((exp.X_add_number >> 16) >> 16) & 0xffffffff;
! 	  inst.operands[i].regisimm = 1;
! 	}
!     }
! 
    else if (exp.X_op == O_big
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number > 32
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number <= 64)

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

[Nick Clifton]

Hi Julian,


> This patch fixes some immediate-out-of-range errors for VBIC, VORR etc. 
> on 64-bit hosts. On such hosts, the X_add_number field of expressions 
> will be 64 bits wide. INT_MIN and INT_MAX are used to signify that any 
> immediate may be accepted for parse_immediate: unfortunately on a 64-bit 
> host, 0xff000000 for instance is then interpreted as a positive integer 
> outside that range.

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() ?

> I've just made passing INT_MIN/INT_MAX disable the 
> check instead, though I'm not very fond of that solution.

There appears to be only once place where INT_MAX/INT_MIN are used, so 
maybe it would be cleaner to change that code to pass realistic minimum 
and maximum values ?

>     * config/tc-arm.c (parse_immediate): Handle 64-bit X_add_number
>     case.

That sentence is not quite true.  The change is to skip the range checks 
when the range is INT_MIN -> INT_MAX.


> !       /* If we're on a 64-bit host, then a 64-bit number can be returned using
> ! 	 O_constant.  We have to be careful not to break compilation for
> ! 	 32-bit X_add_number, though.  */
> !       if ((exp.X_add_number & ~0xffffffffl) != 0)
> ! 	{
> !           inst.operands[i].reg = ((exp.X_add_number >> 16) >> 16) & 0xffffffff;
> ! 	  inst.operands[i].regisimm = 1;
> ! 	}

I am a little bit confused here.  How is ((x >> 16) >> 16) different 
from (x >> 32) ?  If not, then why express it that way ?

Cheers
   Nick

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

[Julian Brown]

Nick Clifton wrote:
> Hi Julian,
> 
> 
>> This patch fixes some immediate-out-of-range errors for VBIC, VORR 
>> etc. on 64-bit hosts. On such hosts, the X_add_number field of 
>> expressions will be 64 bits wide. INT_MIN and INT_MAX are used to 
>> signify that any immediate may be accepted for parse_immediate: 
>> unfortunately on a 64-bit host, 0xff000000 for instance is then 
>> interpreted as a positive integer outside that range.
> 
> 
> 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. 
For VORR, VBIC, etc. a large range of values is acceptable for the 
immediate, so validation is done properly later on in assembling the 
instruction rather than here.

>> I've just made passing INT_MIN/INT_MAX disable the check instead, 
>> though I'm not very fond of that solution.
> 
> 
> There appears to be only once place where INT_MAX/INT_MIN are used, so 
> maybe it would be cleaner to change that code to pass realistic minimum 
> and maximum values ?

There's a design flaw in here somewhere with respect to binutils mostly 
using the word size of the host not the target, and this patch probably 
exascerbates the situation by not really being well thought-out enough, 
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. 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?

>> !       /* If we're on a 64-bit host, then a 64-bit number can be 
>> returned using
>> !      O_constant.  We have to be careful not to break compilation for
>> !      32-bit X_add_number, though.  */
>> !       if ((exp.X_add_number & ~0xffffffffl) != 0)
>> !     {
>> !           inst.operands[i].reg = ((exp.X_add_number >> 16) >> 16) & 
>> 0xffffffff;
>> !       inst.operands[i].regisimm = 1;
>> !     }
> 
> 
> 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). Is there a less weird way of writing the same 
thing?

Cheers,

Julian

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

[Nick Clifton]

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

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

[Daniel Jacobowitz]

On Thu, Jun 15, 2006 at 09:10:01AM +0100, Nick Clifton wrote:
> 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 ?)

No, we do not.  It's standard for GNU tools to only support 32-bit
hosts, even when they support 16-bit targets.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi,

Nick Clifton wrote:
> Hi Julian,
> 
>> 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).

> [...] Or just have the comment in the code.

Here's a (rather belated) new version of the patch which adds 
bounds-checking and non-bounds-checking forms to parse_immediate, 
avoiding the previous nastiness with INT_MIN/INT_MAX.

Tested with "make check" on x86 and x86_64 with cross to arm-none-eabi.

OK? (for mainline? CSL branch?)

Cheers,

Julian

ChangeLog

     gas/
     * config/tc-arm.c (parse_immediate): Add BOUNDED parameter, rename
     to...
     (parse_immediate_maybe_bounded): This. Only bounds-check if BOUNDED
     is true.
     (parse_immediate_bounded): New function, with same arguments and
     semantics as previous parse_immediate.
     (parse_immediate_unbounded): New function. Parse an unbounded
     integer (with sizeof (exp.X_add_number)).
     (parse_big_immediate): Allow for 64-bit exp.X_add_number when
     parsing 64-bit immediates.
     (parse_address_main): Use parse_immediate_bounded not
     parse_immediate.
     (parse_ror): Likewise.
     (parse_operands): Likewise. For Neon immediates, use
     parse_immediate_unbounded. Add new local po_imm_unb_or_fail macro.


[-- Attachment #2: neon-asm-64bit-host-3 --]
[-- Type: text/plain, Size: 6347 bytes --]

? gas/config/~tc-arm.c
Index: gas/config/tc-arm.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-arm.c,v
retrieving revision 1.250.2.29
diff -c -p -r1.250.2.29 tc-arm.c
*** gas/config/tc-arm.c	5 Sep 2006 20:23:46 -0000	1.250.2.29
--- gas/config/tc-arm.c	6 Sep 2006 12:59:19 -0000
*************** const pseudo_typeS md_pseudo_table[] =
*** 3943,3955 ****
  
  /* Generic immediate-value read function for use in insn parsing.
     STR points to the beginning of the immediate (the leading #);
!    VAL receives the value; if the value is outside [MIN, MAX]
!    issue an error.  PREFIX_OPT is true if the immediate prefix is
     optional.  */
  
  static int
! parse_immediate (char **str, int *val, int min, int max,
! 		 bfd_boolean prefix_opt)
  {
    expressionS exp;
    my_get_expression (&exp, str, prefix_opt ? GE_OPT_PREFIX : GE_IMM_PREFIX);
--- 3943,3955 ----
  
  /* Generic immediate-value read function for use in insn parsing.
     STR points to the beginning of the immediate (the leading #);
!    VAL receives the value; if the value is outside [MIN, MAX] and BOUNDED is
!    true, it issue an error.  PREFIX_OPT is true if the immediate prefix is
     optional.  */
  
  static int
! parse_immediate_maybe_bounded (char **str, int *val, bfd_boolean bounded,
! 			       int min, int max, bfd_boolean prefix_opt)
  {
    expressionS exp;
    my_get_expression (&exp, str, prefix_opt ? GE_OPT_PREFIX : GE_IMM_PREFIX);
*************** parse_immediate (char **str, int *val, i
*** 3959,3965 ****
        return FAIL;
      }
  
!   if (exp.X_add_number < min || exp.X_add_number > max)
      {
        inst.error = _("immediate value out of range");
        return FAIL;
--- 3959,3965 ----
        return FAIL;
      }
  
!   if (bounded && (exp.X_add_number < min || exp.X_add_number > max))
      {
        inst.error = _("immediate value out of range");
        return FAIL;
*************** parse_immediate (char **str, int *val, i
*** 3969,3974 ****
--- 3969,3987 ----
    return SUCCESS;
  }
  
+ static int
+ parse_immediate_bounded (char **str, int *val, int min, int max,
+ 			 bfd_boolean prefix_opt)
+ {
+   return parse_immediate_maybe_bounded (str, val, TRUE, min, max, prefix_opt);
+ }
+ 
+ static int
+ parse_immediate_unbounded (char **str, int *val, bfd_boolean prefix_opt)
+ {
+   return parse_immediate_maybe_bounded (str, val, FALSE, 0, 0, prefix_opt);
+ }
+ 
  /* Less-generic immediate-value read function with the possibility of loading a
     big (64-bit) immediate, as required by Neon VMOV and VMVN immediate
     instructions. Puts the result directly in inst.operands[i].  */
*************** parse_big_immediate (char **str, int i)
*** 3982,3988 ****
    my_get_expression (&exp, &ptr, GE_OPT_PREFIX_BIG);
  
    if (exp.X_op == O_constant)
!     inst.operands[i].imm = exp.X_add_number;
    else if (exp.X_op == O_big
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number > 32
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number <= 64)
--- 3995,4012 ----
    my_get_expression (&exp, &ptr, GE_OPT_PREFIX_BIG);
  
    if (exp.X_op == O_constant)
!     {
!       inst.operands[i].imm = exp.X_add_number & 0xffffffff;
!       /* If we're on a 64-bit host, then a 64-bit number can be returned using
! 	 O_constant.  We have to be careful not to break compilation for
! 	 32-bit X_add_number, though.  */
!       if ((exp.X_add_number & ~0xffffffffl) != 0)
! 	{
!           /* X >> 32 is illegal if sizeof (exp.X_add_number) == 4.  */
! 	  inst.operands[i].reg = ((exp.X_add_number >> 16) >> 16) & 0xffffffff;
! 	  inst.operands[i].regisimm = 1;
! 	}
!     }
    else if (exp.X_op == O_big
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number > 32
             && LITTLENUM_NUMBER_OF_BITS * exp.X_add_number <= 64)
*************** parse_address_main (char **str, int i, i
*** 4700,4707 ****
        if (skip_past_char (&p, '{') == SUCCESS)
  	{
  	  /* [Rn], {expr} - unindexed, with option */
! 	  if (parse_immediate (&p, &inst.operands[i].imm,
! 			       0, 255, TRUE) == FAIL)
  	    return PARSE_OPERAND_FAIL;
  
  	  if (skip_past_char (&p, '}') == FAIL)
--- 4724,4731 ----
        if (skip_past_char (&p, '{') == SUCCESS)
  	{
  	  /* [Rn], {expr} - unindexed, with option */
! 	  if (parse_immediate_bounded (&p, &inst.operands[i].imm,
! 				       0, 255, TRUE) == FAIL)
  	    return PARSE_OPERAND_FAIL;
  
  	  if (skip_past_char (&p, '}') == FAIL)
*************** parse_ror (char **str)
*** 4972,4978 ****
        return FAIL;
      }
  
!   if (parse_immediate (&s, &rot, 0, 24, FALSE) == FAIL)
      return FAIL;
  
    switch (rot)
--- 4996,5002 ----
        return FAIL;
      }
  
!   if (parse_immediate_bounded (&s, &rot, 0, 24, FALSE) == FAIL)
      return FAIL;
  
    switch (rot)
*************** parse_operands (char *str, const unsigne
*** 5479,5485 ****
  } while (0)
  
  #define po_imm_or_fail(min, max, popt) do {			\
!   if (parse_immediate (&str, &val, min, max, popt) == FAIL)	\
      goto failure;						\
    inst.operands[i].imm = val;					\
  } while (0)
--- 5503,5515 ----
  } while (0)
  
  #define po_imm_or_fail(min, max, popt) do {			\
!   if (parse_immediate_bounded (&str, &val, min, max, popt) == FAIL) \
!     goto failure;						\
!   inst.operands[i].imm = val;					\
! } while (0)
! 
! #define po_imm_unb_or_fail(popt) do {				\
!   if (parse_immediate_unbounded (&str, &val, popt) == FAIL)	\
      goto failure;						\
    inst.operands[i].imm = val;					\
  } while (0)
*************** parse_operands (char *str, const unsigne
*** 5578,5584 ****
              break;
              try_imm:
              /* Immediate gets verified properly later, so accept any now.  */
!             po_imm_or_fail (INT_MIN, INT_MAX, TRUE);
            }
            break;
  
--- 5608,5614 ----
              break;
              try_imm:
              /* Immediate gets verified properly later, so accept any now.  */
!             po_imm_unb_or_fail (TRUE);
            }
            break;
  
*************** parse_operands (char *str, const unsigne
*** 6027,6032 ****
--- 6057,6063 ----
  #undef po_reg_or_fail
  #undef po_reg_or_goto
  #undef po_imm_or_fail
+ #undef po_imm_unb_or_fail
  #undef po_scalar_or_fail
  \f
  /* Shorthand macro for instruction encoding functions issuing errors.  */

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

[Nick Clifton]

Hi Julian,

   Going through my email backlogs and I found this:

>     gas/
>     * config/tc-arm.c (parse_immediate): Add BOUNDED parameter, rename
>     to...
>     (parse_immediate_maybe_bounded): This. Only bounds-check if BOUNDED
>     is true.
>     (parse_immediate_bounded): New function, with same arguments and
>     semantics as previous parse_immediate.
>     (parse_immediate_unbounded): New function. Parse an unbounded
>     integer (with sizeof (exp.X_add_number)).
>     (parse_big_immediate): Allow for 64-bit exp.X_add_number when
>     parsing 64-bit immediates.
>     (parse_address_main): Use parse_immediate_bounded not
>     parse_immediate.
>     (parse_ror): Likewise.
>     (parse_operands): Likewise. For Neon immediates, use
>     parse_immediate_unbounded. Add new local po_imm_unb_or_fail macro.

Are you still waiting for approval for this patch ?  If so, please 
consider it approved.

Cheers
   Nick