Fix unwind info relocation with large code model on AArch64

[vchuravy]

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.

Co-authored-by: Yichao Yu <[e-mail]>

[yuyichao]

Test added.

[peter.smith]

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

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]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

342

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]

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

[yuyichao]

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 .

[yuyichao]

Ping.

[rengolin]

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

[yuyichao]

Sure.

[peter.smith]

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.

[yuyichao]

(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]

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

[yuyichao]

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

[yuyichao]

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]

Except you don't?

[yuyichao]

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]

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

[yuyichao]

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.

[rengolin]

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

[joerg]

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]

Update to current master and consolidate tests

[vchuravy]

move test and rebase

[vchuravy]

Is there anything else to do? We (Julia) have been carrying this patch for a while.

[merge_guards_bot]

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

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

void RuntimeDyldELF::resolveAArch64Relocation(const SectionEntry &Section,
   case ELF::R_AARCH64_PREL32: {
     uint64_t Result = Value + Addend - FinalAddress;
     assert(static_cast<int64_t>(Result) >= INT32_MIN &&
            static_cast<int64_t>(Result) <= UINT32_MAX);
     support::ulittle32_t::ref{TargetPtr} =
         static_cast<uint32_t>(Result & 0xffffffffU);
     break;
   }
+  case ELF::R_AARCH64_PREL64: {
+    uint64_t *TargetPtr =
+        reinterpret_cast<uint64_t *>(Section.getAddressWithOffset(Offset));
+    uint64_t Result = Value + Addend - FinalAddress;
+    support::ulittle64_t::ref{TargetPtr} = Result;
+    break;
+  }
   case ELF::R_AARCH64_CALL26: // fallthrough
   case ELF::R_AARCH64_JUMP26: {
     // Operation: S+A-P. Set Call or B immediate value to bits fff_fffc of the
     // calculation.
     uint64_t BranchImm = Value + Addend - FinalAddress;
     uint32_t TargetValue = support::ulittle32_t::ref{TargetPtr};
 
     // "Check that -2^27 <= result < 2^27".

lib/MC/MCObjectFileInfo.cpp

void MCObjectFileInfo::initELFMCObjectFileInfo(const Triple &T) {
   case Triple::mips:
   case Triple::mipsel:
     FDECFIEncoding = dwarf::DW_EH_PE_sdata4;
     break;
   case Triple::mips64:
   case Triple::mips64el:
     FDECFIEncoding = dwarf::DW_EH_PE_sdata8;
     break;
+  case Triple::aarch64:
+  case Triple::aarch64_be:
   case Triple::x86_64:
     FDECFIEncoding = dwarf::DW_EH_PE_pcrel |
                      ((CMModel == CodeModel::Large) ? dwarf::DW_EH_PE_sdata8
                                                     : dwarf::DW_EH_PE_sdata4);
     break;
   default:
     FDECFIEncoding = dwarf::DW_EH_PE_pcrel | dwarf::DW_EH_PE_sdata4;
     break;