On 3/7/25 3:38 AM, Nick Clifton wrote:
> Hi Guys,
>
> I am applying the attached patch to fix the msp430 assembler's
> checking of 20-bit immediate values. The code used to check
> for negative values less than -(0x7ffff) but this is incorrect
> as it does not allow -524289.
>
> Whilst correcting this, and adding a few extra lines to the MSP430
> testsuite to make sure that the checks work correctly I realised
> that it might not be obvious why positive values up to 0xfffff are
> allowed for a signed immediate field. So I have added in a couple
> of comments explaining about the special native of hex constants
> in immediate fields.
Thanks for taking care of this. FWIW, it looks like it fixed ~50
pre-existing failures in the GCC testsuite.
There's a few new failures, but I'm far from convinced they're due to
this change. One of them has been a relatively flakey test. The other
two are execution tests which I would expect to be unaffected by this
change.
Anyway, I own fixing the GCC issues we discussed, but wanted to say
thanks for owning the binutils side.
jeff
@@ -3584,7 +3584,13 @@ msp430_operands (struct msp430_opcode_s * opcode, char * line)
if (op1.exp.X_op == O_constant)
{
n = op1.exp.X_add_number;
- if (n > 0xfffff || n < - (0x7ffff))
+ /* Strictly speaking the positive value test should be for "n > 0x7ffff"
+ but traditionally when specifying immediates as hex values any valid
+ bit pattern is allowed. Hence "suba #0xfffff, r6" is allowed, and so
+ the positive value test has to be "n > 0xfffff".
+ FIXME: We could pre-parse the expression to find out if it starts with
+ 0x and only then allow positive values > 0x7fffff. */
+ if (n > 0xfffff || n < -0x80000)
{
as_bad (_("expected value of first argument of %s to fit into 20-bits"),
opcode->name);
@@ -6,13 +6,16 @@
[^:]*:10: Warning: no size modifier after period, .w assumed
[^:]*:10: Warning: a NOP might be needed here, before this interrupt state change
[^:]*:11: Error: instruction bis.a does not exist
-[^:]*:16: Warning: a NOP might also be needed here, after the instruction that changed interrupt state
-[^:]*:16: Warning: a NOP might be needed here, before an interrupt enable instruction
-[^:]*:25: Warning: a NOP might be needed here, after an interrupt disable instruction
-[^:]*:26: Warning: a NOP might be needed here, after an interrupt enable instruction
-[^:]*:29: Warning: a NOP might be needed here, after an interrupt disable instruction
-[^:]*:31: Warning: a NOP might be needed here, after an interrupt disable instruction
-[^:]*:32: Warning: a NOP might be needed here, after an interrupt disable instruction
-[^:]*:33: Warning: a NOP might be needed here, after an interrupt disable instruction
-[^:]*:34: Warning: a NOP might be needed here, after an interrupt enable instruction
+[^:]*:14: Warning: a NOP might also be needed here, after the instruction that changed interrupt state
+[^:]*:14: Error: expected value of first argument of adda to fit into 20-bits
+[^:]*:15: Error: expected value of first argument of adda to fit into 20-bits
+[^:]*:22: Error: expected value of first argument of adda to fit into 20-bits
+[^:]*:27: Warning: a NOP might be needed here, before an interrupt enable instruction
+[^:]*:36: Warning: a NOP might be needed here, after an interrupt disable instruction
+[^:]*:37: Warning: a NOP might be needed here, after an interrupt enable instruction
+[^:]*:40: Warning: a NOP might be needed here, after an interrupt disable instruction
+[^:]*:42: Warning: a NOP might be needed here, after an interrupt disable instruction
+[^:]*:43: Warning: a NOP might be needed here, after an interrupt disable instruction
+[^:]*:44: Warning: a NOP might be needed here, after an interrupt disable instruction
+[^:]*:45: Warning: a NOP might be needed here, after an interrupt enable instruction
[^:]*: Warning: a NOP might be needed after the interrupt state change at the end of the file
@@ -10,6 +10,17 @@
mov. r1, r2
bis.a #8, r2
+ ;; Make sure that the range checking gets #imm20 correct.
+ adda #-524289, r12
+ adda #0x180000, r10
+ ;; An immediate of #524288 will not trigger an error because
+ ;; positive values up to 1048575 are allowed. This is because
+ ;; assembler programmers often specify bit patterns as immediate
+ ;; values in hex knowing that they will fit, despite the fact
+ ;; that those same values, when viewed as integers, are out of range.
+ ;; eg: adda 0xfffff, r1
+ adda #1048576, r11
+
;;; FIXME: Add more tests of assembler error detection here.
;; A NOP is needed *before* an EINT instruction.
@@ -17,7 +28,7 @@
nop
;; And *after* a DINT instruction.
dint
-
+
;; Changing interrupt states in two successive instructions
;; might cause an interrupt to be missed. The assembler
;; should warn about this, if the -mz command line option
@@ -35,3 +46,4 @@
;; We will also get a warning if the last instruction in the file
;; changes the interrupt state, since this file could be linked
;; with another that starts with an interrupt change.
+
@@ -230,3 +230,7 @@ Disassembly of section .text:
0+0406 <[^>]*> 84 18 44 11 rpt r4 \{ rrax.a r4 ;
0+040a <[^>]*> 44 18 45 55 rpt #5 \{ rlax.b r5 ;
0+040e <[^>]*> 05 18 46 66 rpt #6 \{ rlcx.a r6 ;
+0+0412 <[^>]*> ac 08 00 00 adda #524288,r12 ;0x80000
+0+0416 <[^>]*> ab 07 ff ff adda #524287,r11 ;0x7ffff
+0+041a <[^>]*> ac 08 00 00 adda #524288,r12 ;0x80000
+0+041e <[^>]*> ab 07 ff ff adda #524287,r11 ;0x7ffff
@@ -281,3 +281,9 @@ foo:
rpt r4 { rrax.a r4
rpt #5 { rlax.b r5
rpt #6 { rlcx.a r6
+
+ ;; Make sure that the range checking gets #imm20 correct.
+ adda #-524288, r12
+ adda #524287, r11
+ adda #-0x80000, r12
+ adda #0x7ffff, r11
@@ -43,3 +43,4 @@ Disassembly of section .text:
0+062 <[^>]*> 3f 40 f0 00 mov #240, r15 ;#0x00f0
0+066 <[^>]*> 30 40 00 00 br #0x0000 ;
0+06a <[^>]*> 92 52 00 02 72 01 add &0x0200,&0x0172 ;0x0200
+
@@ -55,3 +55,5 @@ main:
;; This next instruction triggered a bug which
;; was fixed by a patch to msp430-dis.c on Jan 2, 2004
add &0x200, &0x172
+
+