[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

I think that we still need tests for this.  You should be able to write a test for this similar to the other dyld tests that we have in the tree.



================
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;
----------------
marsupial wrote:
> compnerd wrote:
> > 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__`.
> While it may happen to work out more than not that Sections[0].getLoadAddress() is the lowest address, it is dependent on the SectionAllocator used and is possible that Sections[//N//].getLoadAddress() > Sections[0].getLoadAddress()
> 
> 
> 
Do we have any guarantees that the sections are mapped sequentially?  If that is the case, then this is fine.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:138
+
+    {
+      RelocationValueRef OriginalRelValueRef;
----------------
marsupial wrote:
> compnerd wrote:
> > Why the extra scope?
> Scoping is for the two temporary objects:
> 
> ```
> RelocationValueRef OriginalRelValueRef;
> auto Itr = Stubs.find(OriginalRelValueRef);
> ```
Im not sure that the additional scope is really adding any value.  The slight bit of extension of the lifetime of the iterator doesn't really make much of a difference, nor does the ref.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:145
+
+      StubMap::const_iterator i = Stubs.find(OriginalRelValueRef);
+
----------------
marsupial wrote:
> compnerd wrote:
> > Use `auto`.  I think that you can fold this a bit:
> > 
> >     if (const auto &S = Stubs.find(OriginalRelValueRef)) {
> >     } else {
> >     }
> Moved to **auto**, but your folding doesn't take into account that **Stubs.end()** can be returned from **find**
`Stub` is probably a better than name than `Itr`.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:114
+        writeDeltaReloc(Target, 0, 0);
+      } else
+        writeDeltaReloc(Target, RE.Addend, Value - ImageBase);
----------------
Please use braces around the `else` here as the first case has them.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:131
 
+  std::tuple<uint64_t, uint64_t, uint64_t> 
+  generateRelocationStub(unsigned SectionID, StringRef TargetName,
----------------
Space after the `std::tuple`.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:154-155
+        Section.advanceStubOffset(getMaxStubSize());
+      }
+      else {
+        DEBUG(dbgs() << " Stub function found for " 
----------------
The `else` should be coddled.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:167
+    // writeDeltaReloc(Section.getAddressWithOffset(Offset), 0, StubOffset);
+    // .xdata exception handler's aren't having this though.
+
----------------
Hmm, any chance of having a XFAIL test case for this?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:284-286
+      if (Name == ".pdata") {
         UnregisteredEHFrameSections.push_back(SectionPair.second);
       }
----------------
Not your change, but, the braces aren't necessary here.


https://reviews.llvm.org/D30709