[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Frederich Munch via Phabricator via llvm-commits]

marsupial marked an inline comment 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:
> 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.


https://reviews.llvm.org/D30709