[PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Jesse Taube]

In Binutils 2.37 the ADR instruction has changed
use alternate instructions.

The change causes armv7-m to not boot.

Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
---
 arch/arm/lib/relocate.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
index 14b7f61c1a..22c419534f 100644
--- a/arch/arm/lib/relocate.S
+++ b/arch/arm/lib/relocate.S
@@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
  */
 
 ENTRY(relocate_code)
-	adr	r3, relocate_code
+/*
+ * Binutils doesn't comply with the arm docs for adr in thumb2
+ * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
+ * to remove ambiguity explicitly define the pseudo-instruction
+ */
+	mov r3, pc
+	subs r3, #4
 	ldr	r1, _image_copy_start_ofs
 	add	r1, r3			/* r1 <- Run &__image_copy_start */
 	subs	r4, r0, r1		/* r4 <- Run to copy offset      */
-- 
2.36.1

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On Sun, Jul 10, 2022 at 03:09:53AM -0400, Jesse Taube wrote:

> In Binutils 2.37 the ADR instruction has changed
> use alternate instructions.
> 
> The change causes armv7-m to not boot.
> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> ---
>  arch/arm/lib/relocate.S | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> index 14b7f61c1a..22c419534f 100644
> --- a/arch/arm/lib/relocate.S
> +++ b/arch/arm/lib/relocate.S
> @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
>   */
>  
>  ENTRY(relocate_code)
> -	adr	r3, relocate_code
> +/*
> + * Binutils doesn't comply with the arm docs for adr in thumb2
> + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
> + * to remove ambiguity explicitly define the pseudo-instruction
> + */
> +	mov r3, pc
> +	subs r3, #4
>  	ldr	r1, _image_copy_start_ofs
>  	add	r1, r3			/* r1 <- Run &__image_copy_start */
>  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */

Thanks for posting this.  I'm hoping that perhaps one of our Arm folks
can review this.  Aside from whitespace (which can be fixed up easily),
this looks good and it's good to have a comment explaining why.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Andre Przywara]

On Sun, 10 Jul 2022 03:09:53 -0400
Jesse Taube <mr.bossman075@gmail.com> wrote:

Hi Jesse,

> In Binutils 2.37 the ADR instruction has changed
> use alternate instructions.

Can you elaborate on this? What has changed exactly, and why? Looking at
the commit you mention below I don't see an immediate problem that would
require code changes? Also it speaks of forward references, but this one
is not one?
And I didn't spot any difference between 2.38 and 2.35, at least not in my
isolated test (but I didn't bother to compile a whole stage 1 GCC with
newer binutils yet).

> The change causes armv7-m to not boot.

What does "causes armv7-m to not boot" mean? It compiles fine, but hangs
or crashes?
Can you show the relevant disassembly from both binutils versions?

And from trying to reproduce this minimally, do we need a ".syntax unified"
in the .S file? 

> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> ---
>  arch/arm/lib/relocate.S | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> index 14b7f61c1a..22c419534f 100644
> --- a/arch/arm/lib/relocate.S
> +++ b/arch/arm/lib/relocate.S
> @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
>   */
>  
>  ENTRY(relocate_code)
> -	adr	r3, relocate_code
> +/*
> + * Binutils doesn't comply with the arm docs for adr in thumb2
> + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
> + * to remove ambiguity explicitly define the pseudo-instruction
> + */
> +	mov r3, pc
> +	subs r3, #4

But this will break ARM, won't it? Because it would require to subtract #8?
I mean there is a reason for this adr instruction, because this offset
calculation is best left to the assembler. Not to speak of the fragility
of assuming that the relocate_code label is pointing to the very first
instruction. The ENTRY macro could also insert instructions.

Cheers,
Andre

>  	ldr	r1, _image_copy_start_ofs
>  	add	r1, r3			/* r1 <- Run &__image_copy_start */
>  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Mon, Jul 11, 2022 at 01:57:40PM +0100, Andre Przywara wrote:
> On Sun, 10 Jul 2022 03:09:53 -0400
> Jesse Taube <mr.bossman075@gmail.com> wrote:
> 
> Hi Jesse,
> 
> > In Binutils 2.37 the ADR instruction has changed
> > use alternate instructions.
> 
> Can you elaborate on this? What has changed exactly, and why? Looking at
> the commit you mention below I don't see an immediate problem that would
> require code changes? Also it speaks of forward references, but this one
> is not one?
> And I didn't spot any difference between 2.38 and 2.35, at least not in my
> isolated test (but I didn't bother to compile a whole stage 1 GCC with
> newer binutils yet).

Some further references Jesse provided off-list:

https://stackoverflow.com/questions/59110205/why-do-forward-reference-adr-instructions-assemble-with-even-offsets-in-thumb-co

https://mail.gnu.org/archive/html/bug-binutils/2019-11/msg00187.html

https://sourceware.org/bugzilla/show_bug.cgi?id=25235

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Andre Przywara]

On Mon, 11 Jul 2022 13:57:40 +0100
Andre Przywara <andre.przywara@arm.com> wrote:

Hi,

> On Sun, 10 Jul 2022 03:09:53 -0400
> Jesse Taube <mr.bossman075@gmail.com> wrote:
> 
> Hi Jesse,
> 
> > In Binutils 2.37 the ADR instruction has changed
> > use alternate instructions.
> 
> Can you elaborate on this? What has changed exactly, and why? Looking at
> the commit you mention below I don't see an immediate problem that would
> require code changes? Also it speaks of forward references, but this one
> is not one?
> And I didn't spot any difference between 2.38 and 2.35, at least not in my
> isolated test (but I didn't bother to compile a whole stage 1 GCC with
> newer binutils yet).

OK, so digging a bit deeper I think I have an idea:
With as 2.35 I get:
080007cc <relocate_code>:
 80007cc:   f2af 0304       subw    r3, pc, #4

whereas with 2.38 it's:
080007cc <relocate_code>:
 80007cc:   f2af 0303       subw    r3, pc, #3

the latter looks correct since we compile relocate.S with -mthumb
-mthumb-interwork, so the lowest bit of the *function* address should be
set, to indicate this is a Thumb function. And "ENTRY(relocate_code)"
clearly tells the assembler that relocate_code is a function entry point,
so should carry the instruction set flag in bit 0.
However we don't use the result of "adr" as an argument for a bx call
later, but to calculate some relocation offset, so the bit is getting in
the way.
Without thinking too much about this, wouldn't it help to just always
clear bit 0 in r3?
Or probably better: to have an additional label, which is not marked as a
function entry point?

Cheers,
Andre

> > The change causes armv7-m to not boot.
> 
> What does "causes armv7-m to not boot" mean? It compiles fine, but hangs
> or crashes?
> Can you show the relevant disassembly from both binutils versions?
> 
> And from trying to reproduce this minimally, do we need a ".syntax unified"
> in the .S file? 
> 
> > Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> > ---
> >  arch/arm/lib/relocate.S | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> > index 14b7f61c1a..22c419534f 100644
> > --- a/arch/arm/lib/relocate.S
> > +++ b/arch/arm/lib/relocate.S
> > @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
> >   */
> >  
> >  ENTRY(relocate_code)
> > -	adr	r3, relocate_code
> > +/*
> > + * Binutils doesn't comply with the arm docs for adr in thumb2
> > + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
> > + * to remove ambiguity explicitly define the pseudo-instruction
> > + */
> > +	mov r3, pc
> > +	subs r3, #4
> 
> But this will break ARM, won't it? Because it would require to subtract #8?
> I mean there is a reason for this adr instruction, because this offset
> calculation is best left to the assembler. Not to speak of the fragility
> of assuming that the relocate_code label is pointing to the very first
> instruction. The ENTRY macro could also insert instructions.
> 
> Cheers,
> Andre
> 
> >  	ldr	r1, _image_copy_start_ofs
> >  	add	r1, r3			/* r1 <- Run &__image_copy_start */
> >  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */
> 

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Andre Przywara]

On Mon, 11 Jul 2022 15:11:13 +0100
Andre Przywara <andre.przywara@arm.com> wrote:

> On Mon, 11 Jul 2022 13:57:40 +0100
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> Hi,
> 
> > On Sun, 10 Jul 2022 03:09:53 -0400
> > Jesse Taube <mr.bossman075@gmail.com> wrote:
> > 
> > Hi Jesse,
> >   
> > > In Binutils 2.37 the ADR instruction has changed
> > > use alternate instructions.  
> > 
> > Can you elaborate on this? What has changed exactly, and why? Looking at
> > the commit you mention below I don't see an immediate problem that would
> > require code changes? Also it speaks of forward references, but this one
> > is not one?
> > And I didn't spot any difference between 2.38 and 2.35, at least not in my
> > isolated test (but I didn't bother to compile a whole stage 1 GCC with
> > newer binutils yet).  
> 
> OK, so digging a bit deeper I think I have an idea:
> With as 2.35 I get:
> 080007cc <relocate_code>:
>  80007cc:   f2af 0304       subw    r3, pc, #4
> 
> whereas with 2.38 it's:
> 080007cc <relocate_code>:
>  80007cc:   f2af 0303       subw    r3, pc, #3
> 
> the latter looks correct since we compile relocate.S with -mthumb
> -mthumb-interwork, so the lowest bit of the *function* address should be
> set, to indicate this is a Thumb function. And "ENTRY(relocate_code)"
> clearly tells the assembler that relocate_code is a function entry point,
> so should carry the instruction set flag in bit 0.
> However we don't use the result of "adr" as an argument for a bx call
> later, but to calculate some relocation offset, so the bit is getting in
> the way.
> Without thinking too much about this, wouldn't it help to just always
> clear bit 0 in r3?
> Or probably better: to have an additional label, which is not marked as a
> function entry point?

Does that fix it?

diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
index 14b7f61c1a..5102bfabde 100644
--- a/arch/arm/lib/relocate.S
+++ b/arch/arm/lib/relocate.S
@@ -78,7 +78,8 @@ ENDPROC(relocate_vectors)
  */
 
 ENTRY(relocate_code)
-	adr	r3, relocate_code
+relocate_base:
+	adr	r3, relocate_base
 	ldr	r1, _image_copy_start_ofs
 	add	r1, r3			/* r1 <- Run &__image_copy_start */
 	subs	r4, r0, r1		/* r4 <- Run to copy offset      */

Seems to generate the same code with both gas 2.35 and gas 2.38.

Cheers,
Andre

> 
> Cheers,
> Andre
> 
> > > The change causes armv7-m to not boot.  
> > 
> > What does "causes armv7-m to not boot" mean? It compiles fine, but hangs
> > or crashes?
> > Can you show the relevant disassembly from both binutils versions?
> > 
> > And from trying to reproduce this minimally, do we need a ".syntax unified"
> > in the .S file? 
> >   
> > > Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> > > ---
> > >  arch/arm/lib/relocate.S | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> > > index 14b7f61c1a..22c419534f 100644
> > > --- a/arch/arm/lib/relocate.S
> > > +++ b/arch/arm/lib/relocate.S
> > > @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
> > >   */
> > >  
> > >  ENTRY(relocate_code)
> > > -	adr	r3, relocate_code
> > > +/*
> > > + * Binutils doesn't comply with the arm docs for adr in thumb2
> > > + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
> > > + * to remove ambiguity explicitly define the pseudo-instruction
> > > + */
> > > +	mov r3, pc
> > > +	subs r3, #4  
> > 
> > But this will break ARM, won't it? Because it would require to subtract #8?
> > I mean there is a reason for this adr instruction, because this offset
> > calculation is best left to the assembler. Not to speak of the fragility
> > of assuming that the relocate_code label is pointing to the very first
> > instruction. The ENTRY macro could also insert instructions.
> > 
> > Cheers,
> > Andre
> >   
> > >  	ldr	r1, _image_copy_start_ofs
> > >  	add	r1, r3			/* r1 <- Run &__image_copy_start */
> > >  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */  
> >   
> 

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Jesse Taube]

On 7/11/22 08:57, Andre Przywara wrote:
> On Sun, 10 Jul 2022 03:09:53 -0400
> Jesse Taube <mr.bossman075@gmail.com> wrote:
> 
> Hi Jesse,
> 
>> In Binutils 2.37 the ADR instruction has changed
>> use alternate instructions.
> 
> Can you elaborate on this? What has changed exactly, and why? Looking at
> the commit you mention below I don't see an immediate problem that would
> require code changes? Also it speaks of forward references, but this one
> is not one?
> And I didn't spot any difference between 2.38 and 2.35, at least not in my
> isolated test (but I didn't bother to compile a whole stage 1 GCC with
> newer binutils yet).
> 
>> The change causes armv7-m to not boot.
> 
> What does "causes armv7-m to not boot" mean? It compiles fine, but hangs
> or crashes?
well when we reallocate it doesn't copy the whole instruction
so its an invalid instruction and boot loops.
> Can you show the relevant disassembly from both binutils versions?
> 
> And from trying to reproduce this minimally, do we need a ".syntax unified"
> in the .S file? 
Yes its required. sorry i didn't post the test code here...
The test code is as follows
```
.syntax unified
.global bug;
.align 4
bug:

         adr     r3, bug
.size bug, .-bug
.type bug 2; // This changes offset from 4 to 3 in
include/linux/linkage.h:ENDPROC
//arm-linux-gnueabi-as  -march=armv7-m -c -o bug.o bug.S &&
arm-linux-gnueabi-objdump --disassemble=bug bug.o
```
> 
>> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
>> ---
>>  arch/arm/lib/relocate.S | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
>> index 14b7f61c1a..22c419534f 100644
>> --- a/arch/arm/lib/relocate.S
>> +++ b/arch/arm/lib/relocate.S
>> @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
>>   */
>>  
>>  ENTRY(relocate_code)
>> -	adr	r3, relocate_code
>> +/*
>> + * Binutils doesn't comply with the arm docs for adr in thumb2
>> + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
>> + * to remove ambiguity explicitly define the pseudo-instruction
>> + */
>> +	mov r3, pc
>> +	subs r3, #4
> 
> But this will break ARM, won't it? Because it would require to subtract #8?
> I mean there is a reason for this adr instruction, because this offset
> calculation is best left to the assembler. Not to speak of the fragility
> of assuming that the relocate_code label is pointing to the very first
> instruction. The ENTRY macro could also insert instructions.
> 
> Cheers,
> Andre
> 
>>  	ldr	r1, _image_copy_start_ofs
>>  	add	r1, r3			/* r1 <- Run &__image_copy_start */
>>  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */
> 

Re: [PATCH v1] arm: relocate: Replace ADR instruction with non-pseudo-instruction

[Jesse Taube]

On 7/11/22 10:19, Andre Przywara wrote:
> On Mon, 11 Jul 2022 15:11:13 +0100
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> On Mon, 11 Jul 2022 13:57:40 +0100
>> Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Hi,
>>
>>> On Sun, 10 Jul 2022 03:09:53 -0400
>>> Jesse Taube <mr.bossman075@gmail.com> wrote:
>>>
>>> Hi Jesse,
>>>   
>>>> In Binutils 2.37 the ADR instruction has changed
>>>> use alternate instructions.  
>>>
>>> Can you elaborate on this? What has changed exactly, and why? Looking at
>>> the commit you mention below I don't see an immediate problem that would
>>> require code changes? Also it speaks of forward references, but this one
>>> is not one?
>>> And I didn't spot any difference between 2.38 and 2.35, at least not in my
>>> isolated test (but I didn't bother to compile a whole stage 1 GCC with
>>> newer binutils yet).  
>>
>> OK, so digging a bit deeper I think I have an idea:
>> With as 2.35 I get:
>> 080007cc <relocate_code>:
>>  80007cc:   f2af 0304       subw    r3, pc, #4
>>
>> whereas with 2.38 it's:
>> 080007cc <relocate_code>:
>>  80007cc:   f2af 0303       subw    r3, pc, #3
>>
>> the latter looks correct since we compile relocate.S with -mthumb
>> -mthumb-interwork, so the lowest bit of the *function* address should be
>> set, to indicate this is a Thumb function. And "ENTRY(relocate_code)"
>> clearly tells the assembler that relocate_code is a function entry point,
>> so should carry the instruction set flag in bit 0.
>> However we don't use the result of "adr" as an argument for a bx call
>> later, but to calculate some relocation offset, so the bit is getting in
>> the way.
>> Without thinking too much about this, wouldn't it help to just always
>> clear bit 0 in r3?
>> Or probably better: to have an additional label, which is not marked as a
>> function entry point?
> 
> Does that fix it?
> 
> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> index 14b7f61c1a..5102bfabde 100644
> --- a/arch/arm/lib/relocate.S
> +++ b/arch/arm/lib/relocate.S
> @@ -78,7 +78,8 @@ ENDPROC(relocate_vectors)
>   */
>  
>  ENTRY(relocate_code)
> -	adr	r3, relocate_code
> +relocate_base:
> +	adr	r3, relocate_base
>  	ldr	r1, _image_copy_start_ofs
>  	add	r1, r3			/* r1 <- Run &__image_copy_start */
>  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */
> 
> Seems to generate the same code with both gas 2.35 and gas 2.38.

Works thanks. Do you want to submit it or. 
Sorry about me not knowing Assembly for arm very well...

Thanks,
Jesse
> Cheers,
> Andre
> 
>>
>> Cheers,
>> Andre
>>
>>>> The change causes armv7-m to not boot.  
>>>
>>> What does "causes armv7-m to not boot" mean? It compiles fine, but hangs
>>> or crashes?
>>> Can you show the relevant disassembly from both binutils versions?
>>>
>>> And from trying to reproduce this minimally, do we need a ".syntax unified"
>>> in the .S file? 
>>>   
>>>> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
>>>> ---
>>>>  arch/arm/lib/relocate.S | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
>>>> index 14b7f61c1a..22c419534f 100644
>>>> --- a/arch/arm/lib/relocate.S
>>>> +++ b/arch/arm/lib/relocate.S
>>>> @@ -78,7 +78,13 @@ ENDPROC(relocate_vectors)
>>>>   */
>>>>  
>>>>  ENTRY(relocate_code)
>>>> -	adr	r3, relocate_code
>>>> +/*
>>>> + * Binutils doesn't comply with the arm docs for adr in thumb2
>>>> + * from commit d3e52e120b68bf19552743fbc078e0a759f48cb7 onward
>>>> + * to remove ambiguity explicitly define the pseudo-instruction
>>>> + */
>>>> +	mov r3, pc
>>>> +	subs r3, #4  
>>>
>>> But this will break ARM, won't it? Because it would require to subtract #8?
>>> I mean there is a reason for this adr instruction, because this offset
>>> calculation is best left to the assembler. Not to speak of the fragility
>>> of assuming that the relocate_code label is pointing to the very first
>>> instruction. The ENTRY macro could also insert instructions.
>>>
>>> Cheers,
>>> Andre
>>>   
>>>>  	ldr	r1, _image_copy_start_ofs
>>>>  	add	r1, r3			/* r1 <- Run &__image_copy_start */
>>>>  	subs	r4, r0, r1		/* r4 <- Run to copy offset      */  
>>>   
>>
>