Differential D35544
[AArch64] Force relocations for all ADRP instructions Authored by mstorsjo on Jul 18 2017, 4:01 AM.
Details This generalizes an existing fix from ELF to MachO and COFF. Test that an ADRP to a local symbol whose offset is known at assembly time still produces relocations, both for MachO and COFF. Test that an ADRP without a @page modifier on MachO fails (previously it didn't).
Diff Detail
Event TimelineComment Actions I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain. Comment Actions
I'm pretty sure that's a bug, I get the wrong answer when I actually try out this function on iOS: .text
.global _foo
.p2align 12
_foo:
.fill 1023, 4, 0xd503201f
adrp x0, Lthere
Lthere:
retI suspect it hasn't come up on MachO because CodeGen never used ADRP to address objects in the __text section so you need assembly to trigger it. So would you mind putting that function into the base AArch64AsmBackend?
Comment Actions I did a quick test like this: static int the_func(int a);
void *get_func(void) {
return the_func;
}
static int the_func(int a) {
return 2*a;
}and got this: $ clang test.c -c -o test.o -O3 -target arm64-apple-darwin $ llvm-objdump -d -r test.o test.o: file format Mach-O arm64 Disassembly of section __TEXT,__text: ltmp0: 0: 00 00 00 90 adrp x0, #0 0000000000000000: ARM64_RELOC_PAGE21 _the_func 4: 00 00 00 91 add x0, x0, #0 0000000000000004: ARM64_RELOC_PAGEOFF12 _the_func 8: c0 03 5f d6 ret _the_func: c: 00 78 1f 53 lsl w0, w0, #1 10: c0 03 5f d6 ret So it seems to me like it actually does use ADRP, but probably via some other codepath, since it does generate a relocation. But fixing it for handwritten assembly obviously also is worthwhile.
Sure, will do. Comment Actions Actually, when I try to move the function from ELFAArch64AsmBackend to AArch64AsmBackend, I suddenly started getting the following errors when assembling your test snippet: test.s:7:9: error: ADR/ADRP relocations must be GOT relative
adrp x0, Lthere
^
test.s:7:9: error: unknown AArch64 fixup kind!
adrp x0, LthereComment Actions
MachO always splits up the __text section by its global symbols (sorry, I realise I was a bit vague earlier), so it emits a relocation for those reasons. You'd need to convince LLVM to use ADRP for something in the same function to see the effect. I'm not aware of any LLVM or C code that would do that (aside from inline asm, of course).
Ah, interesting! I actually think that's correct behaviour. The MachO adrp always has a textual tag for the kind of relocation you want. For example adrp x0, Lthere@PAGE. So without that function we silently miscompiled invalid code instead of diagnosing it; with your change it survives to the relocation phase to be reported. Technically we should probably diagnose it earlier (AsmParser), but for your purposes just checking that there is an error in the MachO case is probably the right thing to do. Comment Actions
Oh, indeed, I forgot that MachO has a different syntax for these. I'll update with tests for both of these cases. Comment Actions Looks fine to me other than a nit about the MachO test. Unless the fix to that gets weird and you want to confirm it's right you can commit without uploading another revision.
| ||||||||||||||||||||||||||||||