[PATCH] D30709: Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.


================
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:
> > > > 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.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:48
+    assert(Result <= UINT32_MAX && "Relocation overflow");
+    writeBytesUnaligned(Result, Target, 4);
+  }
----------------
This can be confusing, since it always writes 32-bits, and this is in a 64-bit environment.  Can we indicate that in the name of the function?


https://reviews.llvm.org/D30709