[arm] After 2bfee35cb86, "unsupported relocation on symbol"

[DimitryAndric]

Bugzilla Link	44929
Resolution	FIXED
Resolved on	May 06, 2020 11:16
Version	10.0
OS	Windows 2000
Blocks	#43900
CC	@emaste,@zmodem,@MaskRay,@nickdesaulniers,@smithp35

Extended Description

After 2bfee35 ("[MC][ELF] Emit a relocation if target is defined in the same section and is non-local"), the following assembly for arm no longer compiles:

clang -target armv6-gnueabihf -c adr-reloc.s

adr r0, foo
.global foo
foo:

Before:

$ clang-llvmorg-10-init-17163-g241f330d6ba -target armv6-gnueabihf-freebsd13.0 -c adr-reloc.s

$ llvm-objdump -d adr-reloc.o

adr-reloc.o: file format ELF32-arm-little

Disassembly of section .text:

00000000 $a.0:
0: 04 00 4f e2 sub r0, pc, #​4

After:

$ clang-llvmorg-10-init-17164-g2bfee35cb86 -target armv6-gnueabihf -c adr-reloc.s
adr-reloc.s:2:1: error: unsupported relocation on symbol
adr r0, foo
^

This is a regression.

[smithp35]

I suspect that this is because adr is a pseudo instruction (ASM only) that can map to a pc-relative add or sub, a short-ranged instruction not suitable for interposition, this isn't easily handled in MC and IIRC it generates and add instruction that is then modified to sub by a fixup after the result of the expression is known. There is a relocation directive for the immediate of the instruction, but there isn't one that says change the add to the sub.

In this particular case I think we'll need to resolve the fixup at assembly time.

[smithp35]

This is also ClangBuiltLinux/linux#863

This is a bit awkward to solve given the structure of MC.

The adr pseudo-instruction fixup in the example is falling foul of the SymA.getBinding() != ELF::STB_LOCAL.

bool ELFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
const MCAssembler &Asm, const MCSymbol &SA, const MCFragment &FB,
bool InSet, bool IsPCRel) const {
const auto &SymA = cast(SA);
if (IsPCRel) {
assert(!InSet);
if (SymA.getBinding() != ELF::STB_LOCAL)
return false;
}
return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
InSet, IsPCRel);
}

My best thought so far on how to fix this and retain 2bfee35 is to introduce a new fixup flag MCFixupKindInfo::FKF_NOT_INTERPOSABLE that we could add to the various adr fixups. This could then pass a parameter to isSymbolRefDifferenceFullyResolvedImpl so that it would become something like:

bool ELFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
const MCAssembler &Asm, const MCSymbol &SA, const MCFragment &FB,
bool InSet, bool IsPCRel, boolIsNonInterposable) const {
const auto &SymA = cast(SA);
if (IsPCRel && !IsNonInterposable) {
assert(!InSet);
if (SymA.getBinding() != ELF::STB_LOCAL)
return false;
}
return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
InSet, IsPCRel);
}

[DimitryAndric]

If this is really a special case (in the sense that it is not a real mnemonic corresponding to an actual opcode), maybe it is easier to skip attempting to emit relocations for it instead? If I understand correctly, this was the behavior of clang before commit 2bfee35.

(Note that I have no idea how hard it is to make such an exception, or if there are more of these pseudo mnemonics in arm assembly.)

[smithp35]

If this is really a special case (in the sense that it is not a real
mnemonic corresponding to an actual opcode), maybe it is easier to skip
attempting to emit relocations for it instead? If I understand correctly,
this was the behavior of clang before commit 2bfee35.

(Note that I have no idea how hard it is to make such an exception, or if
there are more of these pseudo mnemonics in arm assembly.)

Agreed, we do need to turn off relocations for these fixups. I can think of a couple of ways:
1.) Something like the previous suggestion that adds a flag to the fixup and alters isSymbolRefDifferenceFullyResolvedImpl behaviour.
2.) Redefine a local alias at the same address as the global and redirect the fixup at that. This could be complex as all the fixup has is an arbitrary MCExpr that may involve multiple symbols.

There is at least one more ARM pseudo instruction affected that used to assemble fine:

ldr r0, foo
foo:

This is short-hand for:
ldr r0, [pc, #assembler calculated offset to foo]

there is a relocation for this instruction, although not many linkers implement it due to its short range, and not many assemblers emit a relocation for it.

[MaskRay]

Can https://reviews.llvm.org/D72892 work around the issue?

[MaskRay]

adr r0, foo
.global foo
foo:

Is STB_GLOBAL foo intended? It may be an error of the emitter.

[DimitryAndric]

adr r0, foo
.global foo
foo:

Is STB_GLOBAL foo intended? It may be an error of the emitter.

As far as I understand, yes. The original code is here:
https://github.com/freebsd/freebsd/blob/master/sys/arm/arm/locore-v6.S#L75

which results in:

sys/arm/arm/locore.S:-351:128: error: unsupported relocation on symbol
; mrs r0, cpsr ; and r0, r0, #(0x0000001f) ; teq r0, #(0x0000001a) ; bne 1f ; ; bl hypervisor_stub_vect_install ; mov r0, 0 ; adr r1, hypmode_enabled ; str r0, [r1] ; ; mrs r0, cpsr ; bic r0, r0, #(0x00000>
^

Similar constructs are used in multiple places.

[smithp35]

Yes, that patch would fix this. I've marked it as approved on my side. Looking at the ABI more carefully I think that there are relocations supported in the major Arm linker's for these relocations so I think that if we can implement these in LLD then we've got a chance to remove the workaround later.

[MaskRay]

As far as I understand, yes. The original code is here:

adr r1, hypmode_enabled

It can be argued that the code is using an undesired GNU as "feature" if GNU as does not produce an outstanding relocation. See https://sourceware.org/bugzilla/show_bug.cgi?id=25500 for a similar powerpc32 issue I recently reported.

If sys/arm/arm/locore-v6.S is eventually linked as -shared, without some -Bsymbolic or --dynamic-list or --version-script local: magic , hypmode_enabled will be preemptive. It is incorrect to use such relocations.

A better approach is to use a local alias, e.g.

adr r0, .Lfoo$local
.global foo
.set .Lfoo$local, foo
foo:

I hope the FreeBSD code can be fixed if the intention is not to make the symbol preemptible.

[DimitryAndric]

As far as I understand, yes. The original code is here:

adr r1, hypmode_enabled

It can be argued that the code is using an undesired GNU as "feature" if GNU
as does not produce an outstanding relocation. See
https://sourceware.org/bugzilla/show_bug.cgi?id=25500 for a similar
powerpc32 issue I recently reported.

And in that bug report, Alan Modra says the same as I would say, namely:

"Many relocations should not use global symbols in shared libraries, but
there is a lot of powerpc assembly out there that works well enough in
the way it is used. I'm not inclined to break the powerpc/powerpc64
linux kernel without good reason."

Similarly, since the 'adr' assembly construct has worked as expected for
quite a long time now, even though the expectations might have been
"wrong" from a theoretical point of view, I think it is gratuitous to
break it now, for seemingly very little gain. At least, I think the
only practical gain is nicer code organization and refactoring in MC?

If sys/arm/arm/locore-v6.S is eventually linked as -shared, without some
-Bsymbolic or --dynamic-list or --version-script local: magic ,
hypmode_enabled will be preemptive. It is incorrect to use such relocations.

That will not happen, as this is kernel code which is always linked
statically, and preemption is never done in kernels (at least, not to my
knowledge). It is really a totally different environment from userland
shared libraries and executables.

I asked about the symbols on the FreeBSD IRC channel (#bsdmips on EFnet)
and people more knowledgeable about it stated:

"[The symbol] needs to be global because it's called both from C code
elsewhere and from this asm using pc-relative adr, but in either case
they should only ever resolve to that one same foo function written in
the asm code."

A better approach is to use a local alias, e.g.

adr r0, .Lfoo$local
.global foo
.set .Lfoo$local, foo
foo:

I hope the FreeBSD code can be fixed if the intention is not to make the
symbol preemptible.

I also asked about this, and it seems people don't really like it:

"I suspect comment 9 is almost-right (remember I predicted we'd be
forced to do that by their refusal to change anything). The thing is, I
don't think we should have to pervert our code with ugliness like
.Lsomething$something -- this is hand-written asm and readability
counts."

Another question from there was if there is a way of marking the symbol
as being non-interposable, so this problem could be sidestepped:

"I think a more-correct solution would be that we need some way to flag
the global foo as a non-interposible symbol (if I understand that
concept correctly)."

Do you think that would work?

[MaskRay]

Yes, that patch would fix this. I've marked it as approved on my side. Looking at the ABI more carefully I think that there are relocations supported in the major Arm linker's for these relocations so I think that if we can implement these in LLD then we've got a chance to remove the workaround later.

Peter, I appreciate that you are still paying attention to these issues though your work no longer requires your involvement into these tools (my understanding about your comment on https://reviews.llvm.org/D73542).

My understanding is that adr referencing a global symbol is rare, so we can postpone implementing these rare relocation types until there are more demands.

Similarly, since the 'adr' assembly construct has worked as expected for
quite a long time now, even though the expectations might have been
"wrong" from a theoretical point of view, I think it is gratuitous to
break it now, for seemingly very little gain. At least, I think the
only practical gain is nicer code organization and refactoring in MC?

Not merely for nicer code organization, also for correctness of -fsemantic-interposition.

I believe Alan Modra exaggerated the situation. R_PPC_ADDR16_LO are rare. powerpc32 linked with lld was working fine on https://travis-ci.com/ClangBuiltLinux/continuous-integration until someone made a commit to linux-next breaking it by introducing the relocation referencing __kernel_datapage_offset.

It is definitely not that "lots of code is making use of this GNU as bug and LLVM's integrated assembler has to support it as well".

That will not happen, as this is kernel code which is always linked
statically, and preemption is never done in kernels (at least, not to my
knowledge). It is really a totally different environment from userland
shared libraries and executables.

We can argue that this (STV_DEFAULT+STB_GLOBAL is preemptive) is a not-nicely designed feature in ELF. Unfortunately it can no longer be fixed. GCC's foo.localalias and clang's .Lfoo$localalias are introduced to make the intention explicit.

I am also on #bsdmips @ efnet. Said a hello to you:)

[smithp35]

Yes, that patch would fix this. I've marked it as approved on my side. Looking at the ABI more carefully I think that there are relocations supported in the major Arm linker's for these relocations so I think that if we can implement these in LLD then we've got a chance to remove the workaround later.

Peter, I appreciate that you are still paying attention to these issues
though your work no longer requires your involvement into these tools (my
understanding about your comment on https://reviews.llvm.org/D73542).

Yes, I'm now back at Arm; the TCWG group in Linaro will still work on LLD when needed, but they are probably not monitoring it proactively right now. I can still work on LLD I'll have to make sure it doesn't affect other responsibilities. In this particular case there was a Sunday with bad weather so I've implemented the Thumb pc-relative relocations as:
https://reviews.llvm.org/D75039 (MC)
https://reviews.llvm.org/D75042 (LLD)

Thumb is probably not as popular as Arm for assembly but the relocations and instructions are more fiddly than Arm so I thought I'd get the harder ones done first.

My guess is that BSD will be using Arm state so these patches won't be immediately helpful. I should be able to follow up with Arm state patches later in the week or the weekend.

My understanding is that adr referencing a global symbol is rare, so we
can postpone implementing these rare relocation types until there are more
demands.

It is difficult to know. We've got at least one in Linux and at least one in BSD. There could be more in embedded systems code that we don't tend to see upstream.

[zmodem]

What's the status here relative to the 10.0.0 release?

[smithp35]

I think that there is some contention. There are 5 possibilities:
1.) These instructions are rare enough to global variables, and there is a workaround to use a local alias. Therefore won't fix.
2.) There is a patch https://reviews.llvm.org/D72892 that could be applied, this essentially forces the ARM pseudo instruction fixups to be resolved at assembly time.
3.) We could map the fixups to ARM relocations, this would fix the assembler error, but we'd fail at link time with LLD as it doesn't support these relocations yet, although the binutils linkers do.
4.) Map to fixups to ARM relocations as in 3 and implement the relocations in LLD. I'm working on this and have Thumb up for review so far:
https://reviews.llvm.org/D75039 Thumb (MC)
https://reviews.llvm.org/D75042 Thumb (LLD)
I've written the code for the ARM equivalents, but I need to write some tests before submitting. The ARM ADR instruction is complicated as it uses the modified immediate addressing mode where the immediate is an 8-bit constant rotated right by an even 4-bit field.
5.) Revert 2bfee35 on the branch, this is simpler than https://reviews.llvm.org/D72892 but loses the benefits of it.

My personal preference is for 2.) for 10.0 and 4.) for 11.0, superseding D72892. I can probably finish the ARM relocations by the end of the week, but it is quite a bit of fiddly code that I wouldn't want to rush into 10.0. There are differences of opinion though.

[DimitryAndric]

...

2.) There is a patch https://reviews.llvm.org/D72892 that could be applied,
this essentially forces the ARM pseudo instruction fixups to be resolved at
assembly time.

This would also be my preference. Even though the directive may have improperly designed, it is still widely used, and its meaning, though apparently a little fuzzy, seems to be quite well-established. So from an end-user perspective, their hand-written arm assembly code should just keep on working.