[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:36-43
+  uint64_t getImageBase() {
+    if (!ImageBase) {
+      ImageBase = std::numeric_limits<uint64_t>::max();
+      for (const SectionEntry &Section : Sections)
+        ImageBase = std::min(ImageBase, Section.getLoadAddress());
+    }
+    return ImageBase;
----------------
This seems wrong.  We must have an `__ImageBase__` in COFF land.  This would attempt to emulate that, resulting in the `IMAGE_REL_AMD64_ADDR32NB` becoming a `IMAGE_REL_AMD64_SECREL`.

Note that you want `Sections[0].getLoadAddress()` as an approximation of `__ImageBase__`.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:138
+
+    {
+      RelocationValueRef OriginalRelValueRef;
----------------
Why the extra scope?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:145
+
+      StubMap::const_iterator i = Stubs.find(OriginalRelValueRef);
+
----------------
Use `auto`.  I think that you can fold this a bit:

    if (const auto &S = Stubs.find(OriginalRelValueRef)) {
    } else {
    }


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:156
+        Section.advanceStubOffset(getMaxStubSize());
+      }
+      else {
----------------
Remove the newline.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:288
+      // with an IMAGE_REL_AMD64_ADDR32NB relocation. Using a memory manager
+      // that keeps sections ordered in realtion to __ImageBase is necessary.
+      if (Name == ".pdata") {
----------------
Typo `relation` -> `relation`.


https://reviews.llvm.org/D30709