This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols.
ClosedPublic

Authored by grimar on Nov 9 2018, 7:22 AM.

Diff Detail

Event Timeline

grimar created this revision.Nov 9 2018, 7:22 AM
grimar added a comment.Nov 9 2018, 7:23 AM

With that, the test case from the https://reviews.llvm.org/D54145 description starts to work correctly.

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

llvm/tools/lld/test/ELF/gotpc-relax.s:27:16: error: DISASM-NEXT: expected string not found in input
# DISASM-NEXT: 20101f: {{.*}} movq 8154(%rip), %rax
               ^
<stdin>:19:2: note: scanning from here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax
 ^
<stdin>:19:25: note: possible intended match here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax

It will be worth double checking to see if this hasn't broken x86? Or am I missing something?

grimar added a comment.Nov 9 2018, 8:49 AM

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

llvm/tools/lld/test/ELF/gotpc-relax.s:27:16: error: DISASM-NEXT: expected string not found in input
# DISASM-NEXT: 20101f: {{.*}} movq 8154(%rip), %rax
               ^
<stdin>:19:2: note: scanning from here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax
 ^
<stdin>:19:25: note: possible intended match here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax

It will be worth double checking to see if this hasn't broken x86? Or am I missing something?

I assumed this should be independent change, so I did not test them together. I'll try to merge them and check the case you are observing. Thanks!

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

I merged this one with D54145 today manually and run the test suite. It does not fail for me.
They are fully independent I beleive, so it is what I expected.
Here is the merged version I got:

I think just something went wrong on your side perhaps?

ruiu added inline comments.Nov 11 2018, 8:44 PM
ELF/Relocations.h
38

Please add comments to the entries you are adding in this patch. We have to add comments to all these enums. These constants are extremely important but has no description at all. As a starter, we shouldn't add new entries without comments.

grimar updated this revision to Diff 173625.Nov 12 2018, 1:04 AM
  • Added comments for new R_* entries.

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

I merged this one with D54145 today manually and run the test suite. It does not fail for me.
They are fully independent I beleive, so it is what I expected.
Here is the merged version I got:

I think just something went wrong on your side perhaps?

Yes, comparing the diffs, it looks like I missed out adding R_GOT_PC_PLT to needsPlt and needsGot from D54145. With that I get the x86 test passing. Apologies for the noise.

ruiu added inline comments.Nov 12 2018, 10:52 PM
ELF/InputSection.cpp
608

So, this is not specific to AArch64 and

627

this is? It seems a bit weird.

ruiu added a comment.Nov 12 2018, 10:53 PM

Oh okay, so you have two patches that contain duplicate changes. I'll review the other one first.

grimar added inline comments.Nov 13 2018, 1:17 AM
ELF/InputSection.cpp
627

That is because AArch64 use 2 relocations at the same time:

adrp x8, :got:myfunc               # R_AARCH64_ADR_GOT_PAGE
ldr  x8, [x8, :got_lo12:myfunc] # R_AARCH64_LD64_GOT_LO12

R_AARCH64_ADR_GOT_PAGE has R_GOT_PAGE_PC expression and
R_AARCH64_LD64_GOT_LO12 has R_GOT initially and then
converted to *_PLT expressions with 'toPlt()'

ruiu added inline comments.Nov 13 2018, 1:51 AM
ELF/InputSection.cpp
627

I mean this is actually AArch64 specific, no? It clearly uses getAarch64Page. It should be named R_AARCH64_*.

grimar added inline comments.Nov 13 2018, 1:55 AM
ELF/InputSection.cpp
627

Probably. Then R_GOT_PAGE_PC should also be renamed. I was consistent and only added a suffix _PLT.

ruiu added inline comments.Nov 13 2018, 1:58 AM
ELF/InputSection.cpp
627

Can you rename them in another patch?

grimar updated this revision to Diff 173823.Nov 13 2018, 2:26 AM
  • Rebased, renamed new relocation expression.
ELF/InputSection.cpp
627

I've tested the patch against the original C++ example in D54145 and that works ok. I get a segfault when I remove -fPIC though. It does work correctly when I compile with clang but link with ld.bfd (From binutils 2.25).

I'll track down the segfault and will report back as soon as I can. With luck we are just missing something out rather than there being a problem. I think what we have here is a definite improvement though so we could do further work in a follow up patch.

I've tested the patch against the original C++ example in D54145 and that works ok. I get a segfault when I remove -fPIC though. It does work correctly when I compile with clang but link with ld.bfd (From binutils 2.25).

I'll track down the segfault and will report back as soon as I can. With luck we are just missing something out rather than there being a problem. I think what we have here is a definite improvement though so we could do further work in a follow up patch.

I guess the segfault you are talking about is the same I mentioned here: https://reviews.llvm.org/D54145#1291558.
It reproduced without any of my patched, so seems to be a different issue to fix for AArch64.

Yes it is the same one as reported there. I think you are most likely right that it is independent of this particular change.

peter.smith added a comment.EditedNov 13 2018, 3:47 AM

Tracing the segfault in the example:

extern "C" int myfunc();

int main() {

int (*p)() = &myfunc;

return p();

}

0000000000000000 <main>:
   0:	d10083ff 	sub	sp, sp, #0x20
   4:	a9017bfd 	stp	x29, x30, [sp, #16]
   8:	910043fd 	add	x29, sp, #0x10
   c:	90000008 	adrp	x8, 0 <myfunc>
			c: R_AARCH64_ADR_PREL_PG_HI21	myfunc
  10:	91000108 	add	x8, x8, #0x0
			10: R_AARCH64_ADD_ABS_LO12_NC	myfunc
  14:	b81fc3bf 	stur	wzr, [x29, #-4]
  18:	f90003e8 	str	x8, [sp]
  1c:	f94003e8 	ldr	x8, [sp]
  20:	d63f0100 	blr	x8
  24:	a9417bfd 	ldp	x29, x30, [sp, #16]
  28:	910083ff 	add	sp, sp, #0x20
  2c:	d65f03c0 	ret

The R_AARCH64_ADR_PREL_PG_HI21 is represented by R_PAGE_PC. This is actually correctly changed to R_PLT_PAGE_PC by toPlt and a PLT is generated. Unfortunately the resolution of the relocation is incorrect.

000000000022016c <main>:
  22016c:	d10083ff 	sub	sp, sp, #0x20
  220170:	a9017bfd 	stp	x29, x30, [sp, #16]
  220174:	910043fd 	add	x29, sp, #0x10
  220178:	90000008 	adrp	x8, 220000 <_start>
  22017c:	911d4108 	add	x8, x8, #0x750
// The previous 2 instructions should resolve to 27f750
  220180:	b81fc3bf 	stur	wzr, [x29, #-4]
  220184:	f90003e8 	str	x8, [sp]
  220188:	f94003e8 	ldr	x8, [sp]
  22018c:	d63f0100 	blr	x8
  220190:	a9417bfd 	ldp	x29, x30, [sp, #16]
  220194:	910083ff 	add	sp, sp, #0x20
  220198:	d65f03c0 	ret
  22019c:	00000000 	.inst	0x00000000 ; undefined

...

000000000027f750 <.plt>:
  27f750:	d0000010 	adrp	x16, 281000 <main_arena+0x5c8>
  27f754:	f944f211 	ldr	x17, [x16, #2528]
  27f758:	91278210 	add	x16, x16, #0x9e0
  27f75c:	d61f0220 	br	x17

Not quite sure what has gone wrong here.

It looks like the handling for R_PLT_PAGE_PC is broken. It is handled exactly as R_PAGE_PC which always uses Sym.getVA(A). I think it should be using (Sym.getPltVA() + A). I'll submit a separate patch for this as it is unrelated.

Can we land it, Rui?

I think this looks good. It is likely that US based people will be away for thanks giving so it will be worth pinging Rui next week.

ruiu accepted this revision.Nov 26 2018, 11:11 AM

LGTM

This revision is now accepted and ready to land.Nov 26 2018, 11:11 AM
This revision was automatically updated to reflect the committed changes.