ARM pc-relative loads

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ARM pc-relative loads

Paul Brook
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  <[hidden email]>

        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)
 
Reply | Threaded
Open this post in threaded view
|

Re: ARM pc-relative loads

Richard Earnshaw

On Tue, 2011-05-31 at 15:10 +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.
>
> Tested on arm-none-eabi
> Applied to CVS head
>
> Paul
>
> 2011-05-31  Paul Brook  <[hidden email]>
>
> 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.

Hmm, your patch contains additional things not relevant to this
discussion.

I've been thinking about this problem.  My feeling is that, strictly, if
a user wants an internal relocation then they should just mark the
symbol as being visibility protected, or private, or make the symbol
really local if that's what it should be.  I'm not convinced that
propagating a bug in the ARM code support to the Thumb support is the
right way forward.

R.


Reply | Threaded
Open this post in threaded view
|

Re: ARM pc-relative loads

Richard Sandiford-5
In reply to this post by Paul Brook
Paul Brook <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: ARM pc-relative loads

Dave Martin-10
In reply to this post by Paul Brook
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  <[hidden email]>
>
> 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)
>  

Reply | Threaded
Open this post in threaded view
|

Re: ARM pc-relative loads

Dave Martin-10
In reply to this post by Paul Brook
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