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