From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3239 invoked by alias); 6 Sep 2006 13:27:55 -0000 Received: (qmail 3229 invoked by uid 22791); 6 Sep 2006 13:27:54 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 06 Sep 2006 13:27:49 +0000 Received: (qmail 28261 invoked from network); 6 Sep 2006 13:27:46 -0000 Received: from unknown (HELO ?10.1.1.4?) (julian@127.0.0.2) by mail.codesourcery.com with ESMTPA; 6 Sep 2006 13:27:46 -0000 Message-ID: <44FECCCA.5040803@codesourcery.com> Date: Wed, 06 Sep 2006 13:27:00 -0000 From: Julian Brown User-Agent: Thunderbird 1.5.0.5 (X11/20060812) MIME-Version: 1.0 To: Nick Clifton CC: binutils@sources.redhat.com, Paul Brook Subject: Re: [PATCH, ARM] Fix out-of-range immediate assembly errors on 64-bit hosts References: <4489AB12.5090102@codesourcery.com> <448D50C2.5060700@redhat.com> <448D650E.7000305@codesourcery.com> <449115D9.4020809@redhat.com> In-Reply-To: <449115D9.4020809@redhat.com> Content-Type: multipart/mixed; boundary="------------010507030005070008070501" Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2006-09/txt/msg00027.txt.bz2 This is a multi-part message in MIME format. --------------010507030005070008070501 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1819 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. --------------010507030005070008070501 Content-Type: text/plain; name="neon-asm-64bit-host-3" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="neon-asm-64bit-host-3" Content-length: 6347 ? 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 /* Shorthand macro for instruction encoding functions issuing errors. */ --------------010507030005070008070501--