[RuntimeDyld, PowerPC] Fix check for external symbols when detecting reloction overflow

[uweigand]

Summary

The PowerPC part of processRelocationRef currently assumes that external symbols can be identified by checking for SymType == SymbolRef::ST_Unknown. This is actually incorrect in some cases, causing relocation overflows to be mis-detected. The correct check is to test whether Value.SymbolName is null.

Fixes PR32650.

[hfinkel]

LGTM

[hfinkel]

Also, can we have a test case for this?

[uweigand]

Added test case. Note that it is a bit tricky to replicate the exact condition that triggers the bug in a test case. The one attached here seems to fail reliably (before the fix) across different operating system versions on Power, but it still makes a few assumptions (called out in the comments).

Note also that no MCJIT or OrcMCJIT tests were actually run on ppc64le at all -- I've had to add the platform name to the supported list in the lit.local.cfg files in those directories ...

[hfinkel]

LGTM, thanks!

[uweigand]

This seems to have broken some MCJIT test cases on big-endian ppc64 ... I'll have a look.

[uweigand]

It seems that to identify external symbols, we need to check for *either* non-null Value.SymbolName *or* a SymType of Symbol::ST_Unknown.

The former may happen for symbols not known to the JIT at all (e.g. defined in a native library), while the latter happens for symbols known to the JIT, but defined in a different module.

I've checked in a fix as r303655 now, this fixes the regressions on my ppc64 machine.

llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

else
       addRelocationForSection(RE, Value.SectionID);
   } else if (Arch == Triple::ppc64 || Arch == Triple::ppc64le) {
     if (RelType == ELF::R_PPC64_REL24) {
       // Determine ABI variant in use for this object.
       unsigned AbiVariant;
       Obj.getPlatformFlags(AbiVariant);
       AbiVariant &= ELF::EF_PPC64_ABI;
       // A PPC branch relocation will need a stub function if the target is
-      // an external symbol (Symbol::ST_Unknown) or if the target address
+      // an external symbol (Value.SymbolName set) or if the target address
       // is not within the signed 24-bits branch address.
       SectionEntry &Section = Sections[SectionID];
       uint8_t *Target = Section.getAddressWithOffset(Offset);
       bool RangeOverflow = false;
-      if (SymType != SymbolRef::ST_Unknown) {
+      if (!Value.SymbolName) {
         if (AbiVariant != 2) {
           // In the ELFv1 ABI, a function call may point to the .opd entry,
           // so the final symbol value is calculated based on the relocation
           // values in the .opd section.
           if (auto Err = findOPDEntrySection(Obj, ObjSectionToID, Value))
             return std::move(Err);
         } else {
           // In the ELFv2 ABI, a function symbol may provide a local entry
           // point, which must be used for direct calls.
           uint8_t SymOther = Symbol->getOther();
           Value.Addend += ELF::decodePPC64LocalEntryOffset(SymOther);
         }
         uint8_t *RelocTarget =
             Sections[Value.SectionID].getAddressWithOffset(Value.Addend);
         int64_t delta = static_cast<int64_t>(Target - RelocTarget);
         // If it is within 26-bits branch range, just set the branch target
         if (SignExtend64<26>(delta) == delta) {
           RelocationEntry RE(SectionID, Offset, RelType, Value.Addend);
-          if (Value.SymbolName)
-            addRelocationForSymbol(RE, Value.SymbolName);
-          else
-            addRelocationForSection(RE, Value.SectionID);
         } else {
           RangeOverflow = true;
         }
       }
-      if (SymType == SymbolRef::ST_Unknown || RangeOverflow) {
+      if (Value.SymbolName || RangeOverflow) {
         // It is an external symbol (SymbolRef::ST_Unknown) or within a range
         // larger than 24-bits.
         StubMap::const_iterator i = Stubs.find(Value);
         if (i != Stubs.end()) {
           // Symbol function stub already created, just relocate to it
           resolveRelocation(Section, Offset,
                             reinterpret_cast<uint64_t>(
                                 Section.getAddressWithOffset(i->second)),
if (RelType == ELF::R_PPC64_REL24) {
           }
 
           resolveRelocation(Section, Offset, reinterpret_cast<uint64_t>(
                                                  Section.getAddressWithOffset(
                                                      Section.getStubOffset())),
                             RelType, 0);
           Section.advanceStubOffset(getMaxStubSize());
         }
-        if (SymType == SymbolRef::ST_Unknown) {
+        if (Value.SymbolName) {
           // Restore the TOC for external calls
           if (AbiVariant == 2)
             writeInt32BE(Target + 4, 0xE8410018); // ld r2,28(r1)
           else
             writeInt32BE(Target + 4, 0xE8410028); // ld r2,40(r1)
         }
       }
     } else if (RelType == ELF::R_PPC64_TOC16 ||

llvm/trunk/test/ExecutionEngine/MCJIT/lit.local.cfg

 root = config.root
 targets = root.targets
 if ('X86' in targets) | ('AArch64' in targets) | ('ARM' in targets) | \
    ('Mips' in targets) | ('PowerPC' in targets) | ('SystemZ' in targets):
     config.unsupported = False
 else:
     config.unsupported = True
 
 # FIXME: autoconf and cmake produce different arch names. We should normalize
 # them before getting here.
 if root.host_arch not in ['i386', 'x86', 'x86_64', 'AMD64',
-                          'AArch64', 'ARM', 'Mips', 'PowerPC', 'ppc64', 'SystemZ']:
+                          'AArch64', 'ARM', 'Mips',
+                          'PowerPC', 'ppc64', 'ppc64le', 'SystemZ']:
     config.unsupported = True
 
 if 'armv7' in root.host_arch:
     config.unsupported = False
 
 if 'i386-apple-darwin' in root.target_triple:
     config.unsupported = True
 
 if 'powerpc' in root.target_triple and not 'powerpc64' in root.target_triple:
     config.unsupported = True
 
 # ExecutionEngine tests are not expected to pass in a cross-compilation setup.
 if 'native' not in config.available_features:
     config.unsupported = True

llvm/trunk/test/ExecutionEngine/OrcMCJIT/lit.local.cfg

 root = config.root
 targets = root.targets
 if ('X86' in targets) | ('AArch64' in targets) | ('ARM' in targets) | \
    ('Mips' in targets) | ('PowerPC' in targets) | ('SystemZ' in targets):
     config.unsupported = False
 else:
     config.unsupported = True
 
 # FIXME: autoconf and cmake produce different arch names. We should normalize
 # them before getting here.
 if root.host_arch not in ['i386', 'x86', 'x86_64', 'AMD64',
-                          'AArch64', 'ARM', 'Mips', 'PowerPC', 'ppc64', 'SystemZ']:
+                          'AArch64', 'ARM', 'Mips',
+                          'PowerPC', 'ppc64', 'ppc64le', 'SystemZ']:
     config.unsupported = True
 
 if 'armv7' in root.host_arch:
     config.unsupported = False
 
 if 'i386-apple-darwin' in root.target_triple:
     config.unsupported = True
 
 if 'powerpc' in root.target_triple and not 'powerpc64' in root.target_triple:
     config.unsupported = True
 
 # ExecutionEngine tests are not expected to pass in a cross-compilation setup.
 if 'native' not in config.available_features:
     config.unsupported = True

llvm/trunk/test/ExecutionEngine/OrcMCJIT/pr32650.ll

+; RUN: %lli -jit-kind=orc-mcjit %s
+
+; This test is intended to verify that a function weakly defined in
+; JITted code, and strongly defined in the main executable, can be
+; correctly resolved when called from elsewhere in JITted code.
+
+; This test makes the assumption that the lli executable in compiled
+; to export symbols (e.g. --export-dynamic), and that is actually does
+; contain the symbol LLVMInitializeCodeGen.  (Note that this function
+; is not actually called by the test.  The test simply verifes that
+; the reference can be resolved without relocation errors.)
+
+define linkonce_odr void @LLVMInitializeCodeGen() {
+entry:
+  ret void
+}
+
+define void @test() {
+entry:
+  call void @LLVMInitializeCodeGen()
+  ret void
+}
+
+define i32 @main() {
+entry:
+  ret i32 0
+}
+