ARM pc-relative loads

ARM pc-relative loads

[Paul Brook]

[-- Attachment #1: Type: Text/Plain, Size: 1113 bytes --]

The patch below fixes a failure observed building a hacked-up variant of the 
linux kernel.

The assembler is failing to resolve a pc-relative Thumb load from a global 
symbol in the same section.  My first guess was that this is a feature - 
symbol preemption requires the address be resolved at dynamic link time.  
However the offset range of pc-relative load instructions is sufficiently 
small that I don't believe that symbol preemption would ever actually succeed 
in practice.

We already resolve the equivalent ARM relocation. Arguably this is a bug. 
However there is code that relies on this behavior. Given the uselessness of 
exporting these relocations I've chosen to go for consistency with ARM LDR, 
and resolve the relocation.  Further investigation revealed a handful of other 
load instructions that also need to be handled.

Tested on arm-none-eabi
Applied to CVS head

Paul

2011-05-31  Paul Brook  <paul@codesourcery.com>

	gas/
	* config/tc-arm.c (arm_force_relocation): Resolve all pc-relative
	loads.

	gas/testsuite/
	* gas/arm/ldr-global.d: New test.
	* gas/arm/ldr-global.s: New test.

[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 3706 bytes --]

diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
index a9839cd..5430367 100644
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -22769,6 +22769,8 @@ static const struct arm_cpu_option_table arm_cpus[] =
   {"cortex-r4",		ARM_ARCH_V7R,	 FPU_NONE,	  "Cortex-R4"},
   {"cortex-r4f",	ARM_ARCH_V7R,	 FPU_ARCH_VFP_V3D16,
   							  "Cortex-R4F"},
+  {"cortex-r5",		ARM_ARCH_V7R_IDIV,
+					 FPU_NONE,	  "Cortex-R5"},
   {"cortex-m4",		ARM_ARCH_V7EM,	 FPU_NONE,	  "Cortex-M4"},
   {"cortex-m3",		ARM_ARCH_V7M,	 FPU_NONE,	  "Cortex-M3"},
   {"cortex-m1",		ARM_ARCH_V6SM,	 FPU_NONE,	  "Cortex-M1"},
@@ -22852,7 +22854,7 @@ struct arm_option_extension_value_table
 static const struct arm_option_extension_value_table arm_extensions[] =
 {
   {"idiv",	ARM_FEATURE (ARM_EXT_ADIV | ARM_EXT_DIV, 0),
-				   ARM_FEATURE (ARM_EXT_V7A, 0)},
+				   ARM_FEATURE (ARM_EXT_V7A | ARM_EXT_V7R, 0)},
   {"iwmmxt",	ARM_FEATURE (0, ARM_CEXT_IWMMXT),	ARM_ANY},
   {"iwmmxt2",	ARM_FEATURE (0, ARM_CEXT_IWMMXT2),	ARM_ANY},
   {"maverick",	ARM_FEATURE (0, ARM_CEXT_MAVERICK),	ARM_ANY},
diff --git a/gas/doc/c-arm.texi b/gas/doc/c-arm.texi
index a5a88ba..131f6ab 100644
--- a/gas/doc/c-arm.texi
+++ b/gas/doc/c-arm.texi
@@ -152,7 +152,7 @@ been added, again in ascending alphabetical order.  For example,
 
 
 The following extensions are currently supported:
-@code{idiv}, (Integer Divide Extensions for v7-A architecture),
+@code{idiv}, (Integer Divide Extensions for v7-A and v7-R architectures),
 @code{iwmmxt},
 @code{iwmmxt2},
 @code{maverick},
diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.d b/gas/testsuite/gas/arm/arm-idiv-bad.d
new file mode 100644
index 0000000..c3d7394
--- /dev/null
+++ b/gas/testsuite/gas/arm/arm-idiv-bad.d
@@ -0,0 +1,4 @@
+#name: Invalid V7 ARM DIV instructions
+#as: -march=armv7-a
+#error-output: arm-idiv-bad.l
+
diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.l b/gas/testsuite/gas/arm/arm-idiv-bad.l
new file mode 100644
index 0000000..6662cc7
--- /dev/null
+++ b/gas/testsuite/gas/arm/arm-idiv-bad.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+[^:]*:4: Error: selected processor does not support ARM mode `sdiv r0,r0,r0'
diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.s b/gas/testsuite/gas/arm/arm-idiv-bad.s
new file mode 100644
index 0000000..45e846e
--- /dev/null
+++ b/gas/testsuite/gas/arm/arm-idiv-bad.s
@@ -0,0 +1,4 @@
+	.text
+	.arm
+label:
+	sdiv	r0, r0, r0
diff --git a/gas/testsuite/gas/arm/arm-idiv.d b/gas/testsuite/gas/arm/arm-idiv.d
new file mode 100644
index 0000000..a886d02
--- /dev/null
+++ b/gas/testsuite/gas/arm/arm-idiv.d
@@ -0,0 +1,9 @@
+#name: ARM Integer division instructions
+#objdump: -dr --prefix-addresses --show-raw-insn
+
+.*: +file format .*arm.*
+
+Disassembly of section .text:
+0+000 <[^>]*> e735f819 	udiv	r5, r9, r8
+0+004 <[^>]*> e739f715 	udiv	r9, r5, r7
+0+008 <[^>]*> e710f010 	sdiv	r0, r0, r0
diff --git a/gas/testsuite/gas/arm/arm-idiv.s b/gas/testsuite/gas/arm/arm-idiv.s
new file mode 100644
index 0000000..626a67d
--- /dev/null
+++ b/gas/testsuite/gas/arm/arm-idiv.s
@@ -0,0 +1,8 @@
+.arch armv7-a
+.arch_extension idiv
+.arm
+udiv r5, r9, r8
+.cpu cortex-a15
+udiv r9, r5, r7
+.cpu cortex-r5
+sdiv r0, r0, r0
diff --git a/include/opcode/arm.h b/include/opcode/arm.h
index 297ca63..86e3d67 100644
--- a/include/opcode/arm.h
+++ b/include/opcode/arm.h
@@ -229,6 +229,8 @@
 			ARM_FEATURE (ARM_AEXT_V7A | ARM_EXT_MP | ARM_EXT_SEC \
 				     | ARM_EXT_DIV | ARM_EXT_ADIV \
 				     | ARM_EXT_VIRT, 0)
+/* v7-r+idiv.  */
+#define ARM_ARCH_V7R_IDIV	ARM_FEATURE (ARM_AEXT_V7R | ARM_EXT_ADIV, 0)
 /* Features that are present in v6M and v6S-M but not other v6 cores.  */
 #define ARM_ARCH_V6M_ONLY ARM_FEATURE (ARM_AEXT_V6M_ONLY, 0)
 

Re: ARM pc-relative loads

[Richard Sandiford]

Paul Brook <paul@codesourcery.com> writes:
> The patch below fixes a failure observed building a hacked-up variant of the 
> linux kernel.
>
> The assembler is failing to resolve a pc-relative Thumb load from a global 
> symbol in the same section.  My first guess was that this is a feature - 
> symbol preemption requires the address be resolved at dynamic link time.  
> However the offset range of pc-relative load instructions is sufficiently 
> small that I don't believe that symbol preemption would ever actually succeed 
> in practice.
>
> We already resolve the equivalent ARM relocation. Arguably this is a bug. 
> However there is code that relies on this behavior. Given the uselessness of 
> exporting these relocations I've chosen to go for consistency with ARM LDR, 
> and resolve the relocation.  Further investigation revealed a handful of other 
> load instructions that also need to be handled.

FWIW, just to add to Richard's response, he explicitly rejected the change
in this thread:

    http://sources.redhat.com/ml/binutils/2011-02/msg00398.html
    http://sources.redhat.com/ml/binutils/2011-03/msg00019.html

Richard

Re: ARM pc-relative loads

[Dave Martin]

On Tue, May 31, 2011 at 03:10:22PM +0100, Paul Brook wrote:
> The patch below fixes a failure observed building a hacked-up variant of the 
> linux kernel.
> 
> The assembler is failing to resolve a pc-relative Thumb load from a global 
> symbol in the same section.  My first guess was that this is a feature - 
> symbol preemption requires the address be resolved at dynamic link time.  
> However the offset range of pc-relative load instructions is sufficiently 
> small that I don't believe that symbol preemption would ever actually succeed 
> in practice.

The existing approach for working around this is simply to add local
shadow symbols for the affected loads:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6855/1

This is not elegant, but is likely to be needed for some time until or unless
upstream changes to gas percolate down to everyone's toolchain.

> We already resolve the equivalent ARM relocation. Arguably this is a bug. 
> However there is code that relies on this behavior. Given the uselessness of 
> exporting these relocations I've chosen to go for consistency with ARM LDR, 
> and resolve the relocation.  Further investigation revealed a handful of other 
> load instructions that also need to be handled.
> 
> Tested on arm-none-eabi
> Applied to CVS head
> 
> Paul
> 
> 2011-05-31  Paul Brook  <paul@codesourcery.com>
> 
> 	gas/
> 	* config/tc-arm.c (arm_force_relocation): Resolve all pc-relative
> 	loads.
> 
> 	gas/testsuite/
> 	* gas/arm/ldr-global.d: New test.
> 	* gas/arm/ldr-global.s: New test.

> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index a9839cd..5430367 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -22769,6 +22769,8 @@ static const struct arm_cpu_option_table arm_cpus[] =
>    {"cortex-r4",		ARM_ARCH_V7R,	 FPU_NONE,	  "Cortex-R4"},
>    {"cortex-r4f",	ARM_ARCH_V7R,	 FPU_ARCH_VFP_V3D16,
>    							  "Cortex-R4F"},
> +  {"cortex-r5",		ARM_ARCH_V7R_IDIV,
> +					 FPU_NONE,	  "Cortex-R5"},
>    {"cortex-m4",		ARM_ARCH_V7EM,	 FPU_NONE,	  "Cortex-M4"},
>    {"cortex-m3",		ARM_ARCH_V7M,	 FPU_NONE,	  "Cortex-M3"},
>    {"cortex-m1",		ARM_ARCH_V6SM,	 FPU_NONE,	  "Cortex-M1"},
> @@ -22852,7 +22854,7 @@ struct arm_option_extension_value_table
>  static const struct arm_option_extension_value_table arm_extensions[] =
>  {
>    {"idiv",	ARM_FEATURE (ARM_EXT_ADIV | ARM_EXT_DIV, 0),
> -				   ARM_FEATURE (ARM_EXT_V7A, 0)},
> +				   ARM_FEATURE (ARM_EXT_V7A | ARM_EXT_V7R, 0)},
>    {"iwmmxt",	ARM_FEATURE (0, ARM_CEXT_IWMMXT),	ARM_ANY},
>    {"iwmmxt2",	ARM_FEATURE (0, ARM_CEXT_IWMMXT2),	ARM_ANY},
>    {"maverick",	ARM_FEATURE (0, ARM_CEXT_MAVERICK),	ARM_ANY},
> diff --git a/gas/doc/c-arm.texi b/gas/doc/c-arm.texi
> index a5a88ba..131f6ab 100644
> --- a/gas/doc/c-arm.texi
> +++ b/gas/doc/c-arm.texi
> @@ -152,7 +152,7 @@ been added, again in ascending alphabetical order.  For example,
>  
>  
>  The following extensions are currently supported:
> -@code{idiv}, (Integer Divide Extensions for v7-A architecture),
> +@code{idiv}, (Integer Divide Extensions for v7-A and v7-R architectures),
>  @code{iwmmxt},
>  @code{iwmmxt2},
>  @code{maverick},
> diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.d b/gas/testsuite/gas/arm/arm-idiv-bad.d
> new file mode 100644
> index 0000000..c3d7394
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/arm-idiv-bad.d
> @@ -0,0 +1,4 @@
> +#name: Invalid V7 ARM DIV instructions
> +#as: -march=armv7-a
> +#error-output: arm-idiv-bad.l
> +
> diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.l b/gas/testsuite/gas/arm/arm-idiv-bad.l
> new file mode 100644
> index 0000000..6662cc7
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/arm-idiv-bad.l
> @@ -0,0 +1,2 @@
> +[^:]*: Assembler messages:
> +[^:]*:4: Error: selected processor does not support ARM mode `sdiv r0,r0,r0'
> diff --git a/gas/testsuite/gas/arm/arm-idiv-bad.s b/gas/testsuite/gas/arm/arm-idiv-bad.s
> new file mode 100644
> index 0000000..45e846e
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/arm-idiv-bad.s
> @@ -0,0 +1,4 @@
> +	.text
> +	.arm
> +label:
> +	sdiv	r0, r0, r0
> diff --git a/gas/testsuite/gas/arm/arm-idiv.d b/gas/testsuite/gas/arm/arm-idiv.d
> new file mode 100644
> index 0000000..a886d02
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/arm-idiv.d
> @@ -0,0 +1,9 @@
> +#name: ARM Integer division instructions
> +#objdump: -dr --prefix-addresses --show-raw-insn
> +
> +.*: +file format .*arm.*
> +
> +Disassembly of section .text:
> +0+000 <[^>]*> e735f819 	udiv	r5, r9, r8
> +0+004 <[^>]*> e739f715 	udiv	r9, r5, r7
> +0+008 <[^>]*> e710f010 	sdiv	r0, r0, r0
> diff --git a/gas/testsuite/gas/arm/arm-idiv.s b/gas/testsuite/gas/arm/arm-idiv.s
> new file mode 100644
> index 0000000..626a67d
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/arm-idiv.s
> @@ -0,0 +1,8 @@
> +.arch armv7-a
> +.arch_extension idiv
> +.arm
> +udiv r5, r9, r8
> +.cpu cortex-a15
> +udiv r9, r5, r7
> +.cpu cortex-r5
> +sdiv r0, r0, r0
> diff --git a/include/opcode/arm.h b/include/opcode/arm.h
> index 297ca63..86e3d67 100644
> --- a/include/opcode/arm.h
> +++ b/include/opcode/arm.h
> @@ -229,6 +229,8 @@
>  			ARM_FEATURE (ARM_AEXT_V7A | ARM_EXT_MP | ARM_EXT_SEC \
>  				     | ARM_EXT_DIV | ARM_EXT_ADIV \
>  				     | ARM_EXT_VIRT, 0)
> +/* v7-r+idiv.  */
> +#define ARM_ARCH_V7R_IDIV	ARM_FEATURE (ARM_AEXT_V7R | ARM_EXT_ADIV, 0)
>  /* Features that are present in v6M and v6S-M but not other v6 cores.  */
>  #define ARM_ARCH_V6M_ONLY ARM_FEATURE (ARM_AEXT_V6M_ONLY, 0)
>  

Re: ARM pc-relative loads

[Dave Martin]

On Tue, May 31, 2011 at 03:10:22PM +0100, Paul Brook wrote:
> The patch below fixes a failure observed building a hacked-up variant of the 
> linux kernel.
> 
> The assembler is failing to resolve a pc-relative Thumb load from a global 
> symbol in the same section.  My first guess was that this is a feature - 
> symbol preemption requires the address be resolved at dynamic link time.  
> However the offset range of pc-relative load instructions is sufficiently 
> small that I don't believe that symbol preemption would ever actually succeed 
> in practice.
> 
> We already resolve the equivalent ARM relocation. Arguably this is a bug. 
> However there is code that relies on this behavior. Given the uselessness of 
> exporting these relocations I've chosen to go for consistency with ARM LDR, 
> and resolve the relocation.  Further investigation revealed a handful of other 
> load instructions that also need to be handled.

Whatever way this issue is finally resolved, we should resolve the behaviour
of pc-relative add and sub (i.e., adr and related instructions) in the
same way.

The behaviour of adr is currently strange in Thumb-2 on trunk:

	adr to a word-aligned global symbol fails (even with .hidden etc.):

tst.s:11: Error: invalid immediate for address calculation (value = 0x00000004)

	However, adr to a halfword-aligned global symbol succeeds, causing a
	32-bit addw encoding to be generated.  Similarly, adr referencing a
	global symbol on any alignment before the reference generates 32-bit
	subw encodings with no error.

	adr to local symbols succeeds in all cases.

Should I report a bug on this, or does it make sense to look at it as part
of your corrent changes?

Cheers
---Dave