[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Martell Malone via Phabricator via llvm-commits]

martell added a comment.

Glad to see this finally get approved :)
@marsupial do you have commit access?



================
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:
> > marsupial wrote:
> > > compnerd wrote:
> > > > 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.
> > > That guarantee has to be provided by the section allocator itself, and https://reviews.llvm.org/D35103 is a test/example of doing that.
> > > 
> > > That said, this patch was done before some error handling code was put in for the JIT, so I'll see if the assert can be promoted to returning an error.
> > This really does belong in `RuntimeDyldCOFF` rather than here.
> Not sure what you mean...
> It's the clients responsibility to enforce any memory layout requirements.
> I think theres a decent case to include an ordered allocator in LLVM itself, but that can/should be a separate review.
I believe compnerd is aware of that and that is with he approved the change but just left the comment.


https://reviews.llvm.org/D30709