[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Frederich Munch via Phabricator via llvm-commits]

marsupial marked 2 inline comments as done.
marsupial added inline comments.


================
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;
----------------
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()





================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:138
+
+    {
+      RelocationValueRef OriginalRelValueRef;
----------------
compnerd wrote:
> Why the extra scope?
Scoping is for the two temporary objects:

```
RelocationValueRef OriginalRelValueRef;
auto Itr = Stubs.find(OriginalRelValueRef);
```


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:145
+
+      StubMap::const_iterator i = Stubs.find(OriginalRelValueRef);
+
----------------
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**


https://reviews.llvm.org/D30709