Page MenuHomePhabricator

Fix unwind info relocation with large code model on AArch64
Needs ReviewPublic

Authored by vchuravy on Dec 9 2016, 1:09 PM.

Details

Summary

Makes sure that the unwind info uses 64bits pcrel relocation if a large code model is specified and handle the corresponding relocation in the ExecutionEngine. This can happen with certain kernel configuration (the same as the one in https://reviews.llvm.org/D27609, found at least on the ArchLinux stock kernel and the one used on https://www.packet.net/) using the builtin JIT memory manager.

Diff Detail

Event Timeline

yuyichao updated this revision to Diff 80941.Dec 9 2016, 1:09 PM
yuyichao retitled this revision from to Fix unwind info relocation with large code model on AArch64.
yuyichao updated this object.
yuyichao added a reviewer: t.p.northover.
yuyichao added a subscriber: llvm-commits.
yuyichao updated this revision to Diff 80951.Dec 9 2016, 1:54 PM
yuyichao updated this object.

Test added.

peter.smith edited edge metadata.Dec 13 2016, 3:45 AM

The relocation calculation looks right, but I'm not convinced about the ulittle64_t with the information I have right now. I suggest that if D27609 is approved this should follow.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
342 ↗(On Diff #80951)

Why is this ulittle64_t? I see a comment in D27609 about causing trouble on big-endian systems, but that doesn't explain why.

On aarch64 big-endian instructions are little-endian but data is big-endian. Relocations that are applied to instructions are always written little endian, but relocations that are written to data should be written in the target endianness. The R_AARCH64_PREL64 is only applied to data so I am not expecting to see ulittle64_t here.

yuyichao added inline comments.Dec 13 2016, 5:20 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
342 ↗(On Diff #80951)

I actually did not think about the support of aarch64_be in relocation (even though I added it to the FDE encoding condition....).

Ref https://reviews.llvm.org/rL284789
The reason for the ulittle*_t is to support running the JIT on big endian system (for tests and for remote JIT). This is also why the x86 relocations above also have the ulittle*_t in them even though there isn't any big endian x86 systems.

I'll check and use ubig*_t on the data part conditionally for aarch64_be in both PR if necessary.

yuyichao updated this revision to Diff 81226.Dec 13 2016, 6:25 AM
yuyichao edited edge metadata.

Use target endian for data relocation. This one actually depends on https://reviews.llvm.org/D27609 now....

yuyichao marked an inline comment as done.Dec 13 2016, 6:25 AM
yuyichao updated this revision to Diff 81674.Dec 15 2016, 3:13 PM
yuyichao updated this object.

https://reviews.llvm.org/D27609 is accepted and committed. Rebased on top of it and then add tests for the BE binary similar to https://reviews.llvm.org/D27609 .

Hi Yichao,

Most of us are on holidays until January, some on the road, with only mobile Internet. Please, ping again around the 8th if no one has reviewed by then.

Cheers,
Renato

I think the code changes and test look fine, but I think you should get this approved by one of the approvers of https://reviews.llvm.org/D6079, which is where the x86_64 large code model change was made along with the justification for it.

My last reservation is if this is the right thing to do on aarch64 (i.e. can both llvm and gnu libunwind handle DW_EH_PE_sdata8), and if so perhaps it should be applied for other 64-bit architectures with large code models that permit text segment sizes > 4Gb.

My is that the Jit makes it more likely that code and exceptions may be placed more than 32-bits apart when the large-code model is used, and that using the large-code model gives intent that larger exception tables will result.

I note that gcc still uses 32-bit sized entries even for AMD64, I'm guessing that this is because a Jit isn't involved and programs that span a > 4gb .text section size are rare or non-existent.

(i.e. can both llvm and gnu libunwind handle DW_EH_PE_sdata8)

Local testing seems fine. Not sure what other tests I can do.

perhaps it should be applied for other 64-bit architectures with large code models that permit text segment sizes > 4Gb.

I was a little surprised to learn that it is not the case..... I'm not at all familiar with other archs or what relocations they needs or how to test those so I'd rather not doing that in this PR....

My is that the Jit makes it more likely that code and exceptions may be placed more than 32-bits apart

Exactly. I only hit this issue with JIT. I assume this isn't a problem for GCC (or any static compilers in general)....

joerg edited edge metadata.Jan 7 2017, 1:10 AM

The MC part is fine. Can't comment on RuntimeDyld.

...... Looks like the relocation part was implemented in a later pull request https://reviews.llvm.org/D28122 that's already committed.........................

yuyichao updated this revision to Diff 83876.Jan 10 2017, 2:37 PM
yuyichao edited edge metadata.

Rebased to only include the code model part....... Hopefully this time someone can comment on the whole PR instead of half at a time ;-p........

joerg added a comment.Jan 10 2017, 3:08 PM

Except you don't?

Except you don't?

Don't what? The patch has currently only 3 files and the change to the RuntimeDyld is gone.
The tests are of course still included.

joerg added a comment.Jan 10 2017, 3:23 PM

I would expect an IR test for this, but at the very least, the tests should be in MC/ ?

I would expect an IR test for this,

Why IR test? This is unwind info specific so both the input and the output are assembly files.

but at the very least, the tests should be in MC/ ?

The test uses rtdyld. I'm fine with putting it wherever though.

Couldn't you have merged the two tests with different run/check lines?

joerg added a comment.Jan 11 2017, 2:27 AM

About IR vs assembler: the test is in principle completely target neutral. Makes it easier to copy for other archs and our testing here is certainly on the weak side.

About location: the test is for the MC infrastructure. It would be nice if it could just dump .eh_frame directly, but we still don't have tool support for that it seems. llc -asm-verbose also don't include anything about the FDE format. We might want to fix that though.

vchuravy commandeered this revision.Jul 5 2018, 3:05 PM
vchuravy added a reviewer: yuyichao.
vchuravy updated this revision to Diff 154328.Jul 5 2018, 3:08 PM
vchuravy edited the summary of this revision. (Show Details)

Update to current master and consolidate tests