Differential D27629
Fix unwind info relocation with large code model on AArch64 Authored by vchuravy on Dec 9 2016, 1:09 PM.
Details 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. Co-authored-by: Yichao Yu <yyc1992@gmail.com>
Diff Detail Event TimelineComment Actions 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.
Comment Actions Use target endian for data relocation. This one actually depends on https://reviews.llvm.org/D27609 now.... Comment Actions 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 . Comment Actions 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, Comment Actions 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. Comment Actions
Local testing seems fine. Not sure what other tests I can do.
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....
Exactly. I only hit this issue with JIT. I assume this isn't a problem for GCC (or any static compilers in general).... Comment Actions ...... Looks like the relocation part was implemented in a later pull request https://reviews.llvm.org/D28122 that's already committed......................... Comment Actions 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........ Comment Actions
Don't what? The patch has currently only 3 files and the change to the RuntimeDyld is gone. Comment Actions I would expect an IR test for this, but at the very least, the tests should be in MC/ ? Comment Actions
Why IR test? This is unwind info specific so both the input and the output are assembly files.
The test uses rtdyld. I'm fine with putting it wherever though. Comment Actions 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. Comment Actions Unit tests: pass. 61802 tests passed, 0 failed and 781 were skipped. clang-tidy: unknown. clang-format: pass. Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml | ||||||||||||
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.