[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 at arm.com> wrote:

> On Mon, 11 Jul 2022 13:57:40 +0100
> Andre Przywara <andre.przywara at arm.com> wrote:
> 
> Hi,
> 
> > On Sun, 10 Jul 2022 03:09:53 -0400
> > Jesse Taube <mr.bossman075 at 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 at 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      */  
> >   
>