[PATCH] D51343: Fix RuntimeDyldCOFFX86_64 handling of image-relative relocations when there are not loaded sections
Andrew Scheidecker via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 26 15:59:35 PDT 2018
AndrewScheidecker updated this revision to Diff 167212.
AndrewScheidecker added a comment.
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 at 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.
Repository:
rL LLVM
https://reviews.llvm.org/D51343
Files:
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s
tools/llvm-rtdyld/llvm-rtdyld.cpp
Index: tools/llvm-rtdyld/llvm-rtdyld.cpp
===================================================================
--- tools/llvm-rtdyld/llvm-rtdyld.cpp
+++ tools/llvm-rtdyld/llvm-rtdyld.cpp
@@ -88,25 +88,30 @@
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."),
@@ -577,7 +582,11 @@
if (LoadAddr &&
*LoadAddr != static_cast<uint64_t>(
reinterpret_cast<uintptr_t>(Tmp->first))) {
- AlreadyAllocated[*LoadAddr] = Tmp->second;
+ // 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);
}
}
Index: test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s
===================================================================
--- test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s
+++ test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s
@@ -1,6 +1,6 @@
# 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;
@@ -18,9 +18,9 @@
.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 at 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)+5-40960000000000
inst2:
mov %ebx, (__constdata at imgrel+5)
Index: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
===================================================================
--- lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
+++ lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
@@ -37,7 +37,13 @@
if (!ImageBase) {
ImageBase = std::numeric_limits<uint64_t>::max();
for (const SectionEntry &Section : Sections)
- ImageBase = std::min(ImageBase, Section.getLoadAddress());
+ // 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;
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51343.167212.patch
Type: text/x-patch
Size: 4178 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180926/0a41bf99/attachment.bin>
More information about the llvm-commits
mailing list