Fix RuntimeDyldCOFFX86_64 handling of image-relative relocations when there are not loaded sections

[AndrewScheidecker]

Summary

I'm not sure if this is the correct fix, but it does make it work for my use.

Without this change, I get some message in stderr about "IMAGE_REL_AMD64_ADDR32NB relocation requires anordered section layout", and I have to rely on my old workaround for IMAGE_REL_AMD64_ADDR32NB.

With this change, the IMAGE_REL_AMD64_ADDR32NB relocations seem to work correctly.

If I print the sections that have a load address of 0, sometimes it's a zero-sized .data or .bss, but usually it's debug sections that aren't loaded with ProcessAllSections=false.

[marsupial]

Just maybe add a comment similar to the last line of the summary

[AndrewScheidecker]

Added a comment

[marsupial]

Thanks.

[AndrewScheidecker]

I don't have commit access; can you commit it?

[AndrewScheidecker]

Ping.

[lhames]

Hi Andrew,

I am happy to commit on your behalf. Do you have an example of some failing assembly though? I would like to add a test case for your fix.

Cheers,
Lang.

[AndrewScheidecker]

I started to write a test, but it turns out that COFF_x86_64_IMGREL.s actually triggers this problem, since it has empty data sections that aren't loaded. It seems to be an accident that the test passes right now: it expects the F@IMGREL operand to be relocated to section_addr(.text)+0, but that should only be true if the image base is 0. I think for that test to be meaningful, it needs to pass an explicit target-addr-start to llvm-rtdyld, and subtract that address from the section_addr it compares against.

Passing an explicit target-addr-start exposes a similar problem to that fixed by this diff, but in remapSectionsAndSymbols: it adds sections that weren't loaded to the AlreadyAllocated map at address 0, and the other sections are mapped following that, so target-addr-start has no effect. Adding a similar *LoadAddr != 0 check to remapSectionsAndSymbols fixes that.

I also ran into another problem: the target-addr-start argument works on Windows, but not Linux! It turns out that it's a known bug that cl::opt<uint64_t> only works on Windows: https://bugs.llvm.org/show_bug.cgi?id=19665. I changed those command-line options to use cl::opt<unsigned long long>, as is done elsewhere in LLVM to work around the problem.

[lhames]

Hi Andrew,

Sorry for the delay. This looks good to me, and thanks very much for taking the time to write a test case and fix the infrastructure!

If you're able to commit then go for it, otherwise I can commit on your behalf.

Cheers,
Lang.

[AndrewScheidecker]

This looks good to me, and thanks very much for taking the time to write a test case and fix the infrastructure!

You're welcome!

If you're able to commit then go for it, otherwise I can commit on your behalf.

Please commit it on my behalf.

[lhames]

Committed in r344995. Thanks again Andrew!

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

private:
   SmallVector<SID, 2> RegisteredEHFrameSections;
   uint64_t ImageBase;
 
   // Fake an __ImageBase pointer by returning the section with the lowest adress
   uint64_t getImageBase() {
     if (!ImageBase) {
       ImageBase = std::numeric_limits<uint64_t>::max();
       for (const SectionEntry &Section : Sections)
+        // The Sections list may contain sections that weren't loaded for
+        // whatever reason: they may be debug sections, and ProcessAllSections
+        // is false, or they may be sections that contain 0 bytes. If the
+        // section isn't loaded, the load address will be 0, and it should not
+        // be included in the ImageBase calculation.
+        if (Section.getLoadAddress() != 0)
           ImageBase = std::min(ImageBase, Section.getLoadAddress());
     }
     return ImageBase;
   }
 
   void write32BitOffset(uint8_t *Target, int64_t Addend, uint64_t Delta) {
     uint64_t Result = Addend + Delta;
     assert(Result <= UINT32_MAX && "Relocation overflow");
     writeBytesUnaligned(Result, Target, 4);

test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s

 # RUN: rm -rf %t && mkdir -p %t
 # RUN: llvm-mc -triple=x86_64-pc-win32 -filetype=obj -o %t/COFF_x86_64_IMGREL.o %s
-# RUN: llvm-rtdyld -triple=x86_64-pc-win32 -verify -check=%s %t/COFF_x86_64_IMGREL.o
+# RUN: llvm-rtdyld -triple=x86_64-pc-win32 -verify -target-addr-start=40960000000000 -check=%s %t/COFF_x86_64_IMGREL.o
 .text
   .def   F;
   .scl  2;
   .type 32;
   .endef
   .globl  __constdata
 
 .section    .rdata, "dr", discard, __constdata
     .align  8
     __constdata:
       .quad 0
 
 .text
   .globl  F
   .align  16, 0x90
 
 F:                                      # @F
-# rtdyld-check: decode_operand(inst1, 3) = section_addr(COFF_x86_64_IMGREL.o, .text)+0
+# rtdyld-check: decode_operand(inst1, 3) = section_addr(COFF_x86_64_IMGREL.o, .text)+0-40960000000000
 inst1:
     mov %ebx, F@IMGREL
-# rtdyld-check: decode_operand(inst2, 3) = section_addr(COFF_x86_64_IMGREL.o, .rdata)+5
+# rtdyld-check: decode_operand(inst2, 3) = section_addr(COFF_x86_64_IMGREL.o, .rdata)[phone]
 inst2:
     mov %ebx, (__constdata@imgrel+5)

tools/llvm-rtdyld/llvm-rtdyld.cpp

MCPU("mcpu",
      cl::value_desc("cpu-name"),
      cl::init(""));
 
 static cl::list<std::string>
 CheckFiles("check",
            cl::desc("File containing RuntimeDyld verifier checks."),
            cl::ZeroOrMore);
 
-static cl::opt<uint64_t>
+// Tracking BUG: 19665
+// http://llvm.org/bugs/show_bug.cgi?id=19665
+//
+// Do not change these options to cl::opt<uint64_t> since this silently breaks
+// argument parsing.
+static cl::opt<unsigned long long>
 PreallocMemory("preallocate",
               cl::desc("Allocate memory upfront rather than on-demand"),
               cl::init(0));
 
-static cl::opt<uint64_t>
+static cl::opt<unsigned long long>
 TargetAddrStart("target-addr-start",
                 cl::desc("For -verify only: start of phony target address "
                          "range."),
                 cl::init(4096), // Start at "page 1" - no allocating at "null".
                 cl::Hidden);
 
-static cl::opt<uint64_t>
+static cl::opt<unsigned long long>
 TargetAddrEnd("target-addr-end",
               cl::desc("For -verify only: end of phony target address range."),
               cl::init(~0ULL),
               cl::Hidden);
 
-static cl::opt<uint64_t>
+static cl::opt<unsigned long long>
 TargetSectionSep("target-section-sep",
                  cl::desc("For -verify only: Separation between sections in "
                           "phony target address space."),
                  cl::init(0),
                  cl::Hidden);
 
 static cl::list<std::string>
 SpecificSectionMappings("map-section",
for (WorklistT::iterator I = Worklist.begin(), E = Worklist.end();
        I != E;) {
     WorklistT::iterator Tmp = I;
     ++I;
     auto LoadAddr = Checker.getSectionLoadAddress(Tmp->first);
 
     if (LoadAddr &&
         *LoadAddr != static_cast<uint64_t>(
                        reinterpret_cast<uintptr_t>(Tmp->first))) {
+      // A section will have a LoadAddr of 0 if it wasn't loaded for whatever
+      // reason (e.g. zero byte COFF sections). Don't include those sections in
+      // the allocation map.
+      if (*LoadAddr != 0)
         AlreadyAllocated[*LoadAddr] = Tmp->second;
       Worklist.erase(Tmp);
     }
   }
 
   // If the -target-addr-end option wasn't explicitly passed, then set it to a
   // sensible default based on the target triple.
   if (TargetAddrEnd.getNumOccurrences() == 0) {
     if (TargetTriple.isArch16Bit())