[GAS,MSP430] Fix relocation overflow when using #lo(EXP) macro

Message ID 20200114110643.1e24dec7@jozef-kubuntu
State New
Headers show
Series
  • [GAS,MSP430] Fix relocation overflow when using #lo(EXP) macro
Related show

Commit Message

Jozef Lawrynowicz Jan. 14, 2020, 11:06 a.m.
The following assembly code generates a relocation overflow if P is in
upper memory (at or above address 0x10000).

> MOV.W #lo (P), R8


> as -ml reloc.s -o reloc.o

> ld -m msp430X reloc.o

> reloc.o: in function `foo':

> (.text+0x2): relocation truncated to fit: R_MSP430X_ABS16 against symbol `P'

>   defined in .bss section in reloc.o


This code is intended to extract the low 16-bits of the address of P, so it is
valid to use a 430 instruction, even though the address of P might be 20-bits in
size (stored in a 32-bit linker symbol).
However, since an R_MSP430X_ABS16 is generated for this instruction, the
20-bit value overflows when used in the 430 instruction which expects a 16-bit
immediate.

The MSPABI[1] states the following:
  MSP430 instructions allow only a 16-bit field. MSP430 instruction relocations
  are not typically checked for overflow. MSP430 instructions can also be used
  on MSP430X,  but the assembler uses different relocations so that overflow
  can be checked.
  
  R_MSP430_ABS16 and R_MSP430_PCR16 are used for MSP430 instructions. With one
  exception (R_MSP430_ABS16 for $LO16), neither is used for instructions on
  MSP430X. The use of R_MSP430_ABS16 for $LO16 for MSP430X is a special case 
  intended to be half of a 32-bit immediate load.
 
This means that a R_MSP430_ABS16 relocation should be used for #lo, even when
assembling for MSP430X. The attached patch fixes that.

Note that the patch also removes the redundant check of "extended_op" from
CHECK_RELOC_MSP430. As stated in the above text from the ABI, 430X relocations
should always be used if the target is 430X (except for #lo/#hi), even if the
instruction is 430. CHECK_RELOC_MSP430 is only actually used for 430
instructions anyway.

Successfully regtested the binutils and GCC/G++ testsuites for msp430-elf
in default configuration and -mlarge/-mdata-region=upper.

Ok to apply?

[1] 11.5.1.4 Relocations for MSP430 Instructions
(http://www.ti.com/lit/an/slaa534/slaa534.pdf)

Comments

Nick Clifton Jan. 15, 2020, 1:05 p.m. | #1
Hi Jozef,

> Ok to apply?

> 

> [1] 11.5.1.4 Relocations for MSP430 Instructions

> (http://www.ti.com/lit/an/slaa534/slaa534.pdf)


Approved - please apply.

Cheers
  Nick

Patch

From fed15a67dc1869e26d17a84f88ac4d027a48316c Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Mon, 13 Jan 2020 17:15:23 +0000
Subject: [PATCH] MSP430: Fix relocation overflow when using #lo(EXP) macro

gas/ChangeLog:

2020-01-14  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* config/tc-msp430.c (CHECK_RELOC_MSP430): Always generate 430X
	relocations when the target is 430X, except when extracting part of an
	expression.
	(msp430_srcoperand): Adjust comment.
	Initialize the expp member of the msp430_operand_s struct as
	appropriate.
	(msp430_dstoperand): Likewise.
	* testsuite/gas/msp430/msp430.exp: Run new test.
	* testsuite/gas/msp430/reloc-lo-430x.d: New test.
	* testsuite/gas/msp430/reloc-lo-430x.s: New test.

include/ChangeLog:

2020-01-14  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* opcode/msp430.h (enum msp430_expp_e): New.
	(struct msp430_operand_s): Add expp member to struct.

ld/ChangeLog:

2020-01-14  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* testsuite/ld-msp430-elf/msp430-elf.exp: Run new test.
	* testsuite/ld-msp430-elf/reloc-lo-430x.s: New test.
---
 gas/config/tc-msp430.c                     | 35 +++++++++++++++-------
 gas/testsuite/gas/msp430/msp430.exp        |  1 +
 gas/testsuite/gas/msp430/reloc-lo-430x.d   |  5 ++++
 gas/testsuite/gas/msp430/reloc-lo-430x.s   | 22 ++++++++++++++
 include/opcode/msp430.h                    | 15 ++++++++++
 ld/testsuite/ld-msp430-elf/msp430-elf.exp  |  2 ++
 ld/testsuite/ld-msp430-elf/reloc-lo-430x.s | 22 ++++++++++++++
 7 files changed, 91 insertions(+), 11 deletions(-)
 create mode 100644 gas/testsuite/gas/msp430/reloc-lo-430x.d
 create mode 100644 gas/testsuite/gas/msp430/reloc-lo-430x.s
 create mode 100644 ld/testsuite/ld-msp430-elf/reloc-lo-430x.s

diff --git a/gas/config/tc-msp430.c b/gas/config/tc-msp430.c
index b627740f50..57538824e9 100644
--- a/gas/config/tc-msp430.c
+++ b/gas/config/tc-msp430.c
@@ -275,21 +275,21 @@  target_is_430xv2 (void)
   return selected_isa == MSP_ISA_430Xv2;
 }
 
-/* Generate an absolute 16-bit relocation.
-   For the 430X we generate a relocation without linker range checking
-    if the value is being used in an extended (ie 20-bit) instruction,
-    otherwise if have a shifted expression we use a HI reloc.
+/* Generate an absolute 16-bit relocation, for 430 (!extended_op) instructions
+   only.
+   For the 430X we generate a 430 relocation only for the case where part of an
+   expression is being extracted (e.g. #hi(EXP), #lo(EXP). Otherwise generate
+   a 430X relocation.
    For the 430 we generate a relocation without assembler range checking
-    if we are handling an immediate value or a byte-width instruction.  */
+   if we are handling an immediate value or a byte-width instruction.  */
 
 #undef  CHECK_RELOC_MSP430
 #define CHECK_RELOC_MSP430(OP)				\
   (target_is_430x ()					\
-  ? (extended_op					\
-     ? BFD_RELOC_16					\
-     : ((OP).vshift == 1)				\
-     ? BFD_RELOC_MSP430_ABS_HI16			\
-     : BFD_RELOC_MSP430X_ABS16)				\
+   ? ((OP).expp == MSP_EXPP_ALL				\
+       ? BFD_RELOC_MSP430X_ABS16			\
+       : ((OP).vshift == 1				\
+	  ? BFD_RELOC_MSP430_ABS_HI16 : BFD_RELOC_16))	\
    : ((imm_op || byte_op)				\
       ? BFD_RELOC_MSP430_16_BYTE : BFD_RELOC_MSP430_16))
 
@@ -1909,13 +1909,15 @@  msp430_srcoperand (struct msp430_operand_s * op,
       char *h = l;
       int vshift = -1;
       int rval = 0;
+      /* Use all parts of the constant expression by default.  */
+      enum msp430_expp_e expp = MSP_EXPP_ALL;
 
       /* Check if there is:
 	 llo(x) - least significant 16 bits, x &= 0xffff
 	 lhi(x) - x = (x >> 16) & 0xffff,
 	 hlo(x) - x = (x >> 32) & 0xffff,
 	 hhi(x) - x = (x >> 48) & 0xffff
-	 The value _MUST_ be constant expression: #hlo(1231231231).  */
+	 The value _MUST_ be an immediate expression: #hlo(1231231231).  */
 
       *imm_op = TRUE;
 
@@ -1923,31 +1925,37 @@  msp430_srcoperand (struct msp430_operand_s * op,
 	{
 	  vshift = 0;
 	  rval = 3;
+	  expp = MSP_EXPP_LLO;
 	}
       else if (strncasecmp (h, "#lhi(", 5) == 0)
 	{
 	  vshift = 1;
 	  rval = 3;
+	  expp = MSP_EXPP_LHI;
 	}
       else if (strncasecmp (h, "#hlo(", 5) == 0)
 	{
 	  vshift = 2;
 	  rval = 3;
+	  expp = MSP_EXPP_HLO;
 	}
       else if (strncasecmp (h, "#hhi(", 5) == 0)
 	{
 	  vshift = 3;
 	  rval = 3;
+	  expp = MSP_EXPP_HHI;
 	}
       else if (strncasecmp (h, "#lo(", 4) == 0)
 	{
 	  vshift = 0;
 	  rval = 2;
+	  expp = MSP_EXPP_LO;
 	}
       else if (strncasecmp (h, "#hi(", 4) == 0)
 	{
 	  vshift = 1;
 	  rval = 2;
+	  expp = MSP_EXPP_HI;
 	}
 
       op->reg = 0;		/* Reg PC.  */
@@ -1956,6 +1964,7 @@  msp430_srcoperand (struct msp430_operand_s * op,
       __tl = h + 1 + rval;
       op->mode = OP_EXP;
       op->vshift = vshift;
+      op->expp = expp;
 
       end = parse_exp (__tl, &(op->exp));
       if (end != NULL && *end != 0 && *end != ')' )
@@ -2167,6 +2176,7 @@  msp430_srcoperand (struct msp430_operand_s * op,
 	}
       op->mode = OP_EXP;
       op->vshift = 0;
+      op->expp = MSP_EXPP_ALL;
       if (op->exp.X_op == O_constant)
 	{
 	  int x = op->exp.X_add_number;
@@ -2275,6 +2285,7 @@  msp430_srcoperand (struct msp430_operand_s * op,
       *h = 0;
       op->mode = OP_EXP;
       op->vshift = 0;
+      op->expp = MSP_EXPP_ALL;
       end = parse_exp (__tl, &(op->exp));
       if (end != NULL && *end != 0)
 	{
@@ -2348,6 +2359,7 @@  msp430_srcoperand (struct msp430_operand_s * op,
   op->am = (*l == '-' ? 3 : 1);
   op->ol = 1;
   op->vshift = 0;
+  op->expp = MSP_EXPP_ALL;
   __tl = l;
   end = parse_exp (__tl, &(op->exp));
   if (end != NULL && * end != 0)
@@ -2382,6 +2394,7 @@  msp430_dstoperand (struct msp430_operand_s * op,
       op->am = 1;
       op->ol = 1;
       op->vshift = 0;
+      op->expp = MSP_EXPP_ALL;
       (void) parse_exp (__tl, &(op->exp));
 
       if (op->exp.X_op != O_constant || op->exp.X_add_number != 0)
diff --git a/gas/testsuite/gas/msp430/msp430.exp b/gas/testsuite/gas/msp430/msp430.exp
index 6eaa659c54..624867f33b 100644
--- a/gas/testsuite/gas/msp430/msp430.exp
+++ b/gas/testsuite/gas/msp430/msp430.exp
@@ -52,4 +52,5 @@  if [expr [istarget "msp430-*-*"]]  then {
     run_dump_test "attr-430x-large-lower-good"
     run_dump_test "attr-430x-large-any-bad"
     run_dump_test "attr-430x-large-any-good"
+    run_dump_test "reloc-lo-430x"
 }
diff --git a/gas/testsuite/gas/msp430/reloc-lo-430x.d b/gas/testsuite/gas/msp430/reloc-lo-430x.d
new file mode 100644
index 0000000000..78adb84490
--- /dev/null
+++ b/gas/testsuite/gas/msp430/reloc-lo-430x.d
@@ -0,0 +1,5 @@ 
+#as: -ml
+#readelf: -r
+#...
+.*R_MSP430_ABS16.*P \+ 0
+#...
diff --git a/gas/testsuite/gas/msp430/reloc-lo-430x.s b/gas/testsuite/gas/msp430/reloc-lo-430x.s
new file mode 100644
index 0000000000..7e9d16bf3a
--- /dev/null
+++ b/gas/testsuite/gas/msp430/reloc-lo-430x.s
@@ -0,0 +1,22 @@ 
+.text
+	.balign 2
+	.global	foo
+	.type	foo, @function
+foo:
+  MOV.W	#lo (P), R8
+	RETA
+	.size	foo, .-foo
+
+	.balign 2
+	.global	main
+	.type	main, @function
+main:
+	CALLA	#foo
+.L4:
+	BRA	#.L4
+	.size	main, .-main
+	.section	.bss,"aw",@nobits
+	.balign 2
+	.global	P
+P:
+	.zero	4
diff --git a/include/opcode/msp430.h b/include/opcode/msp430.h
index aaf0990bf8..96fe674c8f 100644
--- a/include/opcode/msp430.h
+++ b/include/opcode/msp430.h
@@ -21,6 +21,18 @@ 
 #ifndef __MSP430_H_
 #define __MSP430_H_
 
+enum msp430_expp_e
+{
+  MSP_EXPP_ALL = 0,	/* Use full the value of the expression - default.  */
+  MSP_EXPP_LO,		/* Extract least significant word from expression.  */
+  MSP_EXPP_HI,		/* Extract 2nd word from expression.  */
+  MSP_EXPP_LLO,		/* Extract least significant word from an
+			   immediate value.  */
+  MSP_EXPP_LHI,		/* Extract 2nd word from an immediate value.  */
+  MSP_EXPP_HLO,		/* Extract 3rd word from an immediate value.  */
+  MSP_EXPP_HHI,		/* Extract 4th word from an immediate value.  */
+};
+
 struct msp430_operand_s
 {
   int ol;	/* Operand length words.  */
@@ -28,6 +40,9 @@  struct msp430_operand_s
   int reg;	/* Register.  */
   int mode;	/* Operand mode.  */
   int vshift;   /* Number of bytes to shift operand down.  */
+  enum msp430_expp_e expp;	/* For when the operand is a constant
+				   expression, the part of the expression to
+				   extract.  */
 #define OP_REG		0
 #define OP_EXP		1
 #ifndef DASM_SECTION
diff --git a/ld/testsuite/ld-msp430-elf/msp430-elf.exp b/ld/testsuite/ld-msp430-elf/msp430-elf.exp
index c9d4bee3eb..777b358a74 100644
--- a/ld/testsuite/ld-msp430-elf/msp430-elf.exp
+++ b/ld/testsuite/ld-msp430-elf/msp430-elf.exp
@@ -174,6 +174,8 @@  run_ld_link_tests $msp430eithershuffletests
 run_ld_link_tests $msp430warntests
 
 run_dump_test valid-map
+run_ld_link_tests {{ "Check no reloc overflow with #lo and data in the upper region"
+        "-m msp430X" "" "" {reloc-lo-430x.s} {} "reloc-lo-430x"}}
 
 # Don't run data region tests if a data region is specified
 if {[string match "*-mdata-region*" [board_info [target_info name] multilib_flags]]} {
diff --git a/ld/testsuite/ld-msp430-elf/reloc-lo-430x.s b/ld/testsuite/ld-msp430-elf/reloc-lo-430x.s
new file mode 100644
index 0000000000..7e9d16bf3a
--- /dev/null
+++ b/ld/testsuite/ld-msp430-elf/reloc-lo-430x.s
@@ -0,0 +1,22 @@ 
+.text
+	.balign 2
+	.global	foo
+	.type	foo, @function
+foo:
+  MOV.W	#lo (P), R8
+	RETA
+	.size	foo, .-foo
+
+	.balign 2
+	.global	main
+	.type	main, @function
+main:
+	CALLA	#foo
+.L4:
+	BRA	#.L4
+	.size	main, .-main
+	.section	.bss,"aw",@nobits
+	.balign 2
+	.global	P
+P:
+	.zero 4
-- 
2.17.1