Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF

[marsupial]

Summary

IMAGE_REL_AMD64_ADDR32NB relocations are currently set to zero in all cases.
This patch sets the relocation to the correct value when possible and shows an error when not.

[marsupial]

ping

[marsupial]

ping

[martell]

@compnerd would you be in a position to review this?
It seems to be dying a slow death :)
I assume this is important for cling and just in general dynamically loading dll functions.
Something I was previously exploring in rust.
Thanks for the contribution @marsupial.

[compnerd]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

36–43 ↗

(On Diff #90895)

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__.

138 ↗

(On Diff #90895)

Why the extra scope?

145 ↗

(On Diff #90895)

Use auto. I think that you can fold this a bit:

if (const auto &S = Stubs.find(OriginalRelValueRef)) {
} else {
}

156 ↗

(On Diff #90895)

Remove the newline.

288 ↗

(On Diff #90895)

Typo relation -> relation.

[marsupial]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

36–43 ↗

(On Diff #90895)

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

138 ↗

(On Diff #90895)

Scoping is for the two temporary objects:

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

145 ↗

(On Diff #90895)

Moved to auto, but your folding doesn't take into account that Stubs.end() can be returned from find

[marsupial]

Ping.

[marsupial]

Ping.

[marsupial]

Ping.

@compnerd is it possible to restart this.
I added an example to clang that verifies this works in D35103.
Just don't know how to trigger the relocation types in LLVM.

[marsupial]

Ping.

[compnerd]

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.

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

114 ↗

(On Diff #101421)

Please use braces around the else here as the first case has them.

131 ↗

(On Diff #101421)

Space after the std::tuple.

154–155 ↗

(On Diff #101421)

The else should be coddled.

167 ↗

(On Diff #101421)

Hmm, any chance of having a XFAIL test case for this?

284–286 ↗

(On Diff #101421)

Not your change, but, the braces aren't necessary here.

36–43 ↗

(On Diff #90895)

Do we have any guarantees that the sections are mapped sequentially? If that is the case, then this is fine.

138 ↗

(On Diff #90895)

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.

145 ↗

(On Diff #90895)

Stub is probably a better than name than Itr.

[marsupial]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

167 ↗

(On Diff #101421)

As noted before, https://reviews.llvm.org/D35103 is dependent on this and a use case for the changes.
Any hints on how to generate/test the instruction without the clang codebase would be helpful.

36–43 ↗

(On Diff #90895)

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.

[compnerd]

We have tests for the dyld bits in the tree already. You should be able to use that for inspiration for writing the tests. They are in test/ExecutionEngine/RuntimeDyld/<ARCH>

[• LepelTsmok]

//I tried this "patch" with Windows 7 64bit to realize exception handling from jitted code. That works fine.

But if you run the same application on Windows 10, it will crash. I even compiled the code with the windows 10 SDK.//

EDIT:
The problem under windows 10 comes from my code as it looks like. I'm sorry

[marsupial]

Added test case for IMAGE_REL_AMD64_ADDR32NB with JIT and requested formatting changes.

[marsupial]

Ping

[compnerd]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

48 ↗

(On Diff #134486)

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?

36–43 ↗

(On Diff #90895)

This really does belong in RuntimeDyldCOFF rather than here.

[marsupial]

Changed name of function to write32BitOffset.

[marsupial]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

36–43 ↗

(On Diff #90895)

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.

[martell]

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

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

36–43 ↗

(On Diff #90895)

I believe compnerd is aware of that and that is with he approved the change but just left the comment.

[marsupial]

Yes, thanks.
Just waiting till I had a moment to watch the new test on the bots.

[• LepelTsmok]

Hello marsupial,

I know that this revision is closed - but I have a question related to your changes.
I'm using your code for a while now - but there is one behavior I don't understand. I used Visual Studio to generate an object file. The visual studio compiler generated a real strange code, which makes a switch case depending on

__ImageBase

.
However - I use the ExecutionEngine to jit an empty bitcode file, while adding the mentioned object file to the process. Without your code, the execution of the switch case crashes the application. But with your code things are getting interessting. When the ExecutionEngine tries to resolve

__ImageBase

I did the same thing as you: gave him the lowest address of my sections. CRASH! So I decided - just for fun - to gave him the address of a function. Using your code, that function will get called! That is not happening without your code. Also! When I add this object file to the JIT-Process, then exceptions won't work anymore.

I now wonder, if there is a way, that

__ImageBase

won't get resolved by "me" but by your code in the "RuntimeDyldCOFFX86_64.h" file - then everything would be identical and maybe could work.

I would be really happy if I get a response to this and I try my best to explain the situation.

Edit:
I put ImageBase in quotes so it won't trigger underlining

[marsupial]

It's a little hard to understand the issue. To be clear:
This patch is fixing an instance that used to crash?
However there is a bit of manual setup for the JIT you are still not happy with?

Have you looked at setting __ImageBase via RuntimeDyld::mapSectionAddress or DynamicLibrary::AddSymbol
The later would probably require a custom SectionAllocator that pre-allocated so you know the base address before relocation occurs.

Loading an object file that uses __ImageBase might be problematic as the other compiler may be generating sections that the JIT will later discard.
This would make all relocation offsets relative to it incorrect later.

Lastly you had exceptions working between JITed and Non-Jited code in Windows without this patch?
It was specifically submitted to enable that behavior which I could not get otherwise (see https://reviews.llvm.org/D35103).

[• LepelTsmok]

It's a little hard to understand the issue.

I'm sorry! I try my best >o<

This patch is fixing an instance that used to crash?

Yes! Your patch helped me to make exceptions run from my JITed code

Lastly you had exceptions working between JITed and Non-Jited code in Windows without this patch?

No! Never!

Have you looked at setting __ImageBase via RuntimeDyld::mapSectionAddress or DynamicLibrary::AddSymbol
The later would probably require a custom SectionAllocator that pre-allocated so you know the base address before relocation occurs.

Haven't tried this yet

Loading an object file that uses __ImageBase might be problematic as the other compiler may be generating sections that the JIT will later discard.
This would make all relocation offsets relative to it incorrect later.

The strange case is, that this situation never worked.

1. Case:

I'm running the LLVM without your patch. The object file from Visual Studio gets loaded and I call a function from it. I resolved __ImageBase with a function.
-> Crash!

2. Case:

I'm running the LLVM with your patch and do the same.
-> The function (I used to overload __ImageBase) gets called and the application crashes.

I can provide you the object file or source code which generates this specific code. Also I can provide the mixed assembly file I could even record my screen to show what I do.

[marsupial]

I think what you're trying to do just isn't unsupported at this time.
Why not just use the JIT to compile the source for the on disk object at runtime?

Mixing and matching object files from two different compilers (1 on disk from VStudio, and 1 in memory via clang) seems prone to error for a variety of reasons.
Also __ImageBase isn't supposed to be a function it's an IMAGE_DOS_HEADER that is used by the runtime loader.

[• LepelTsmok]

I think what you're trying to do just isn't unsupported at this time.

So it is supported?

Why not just use the JIT to compile the source for the on disk object at runtime?

I will not always have the source code

Also __ImageBase isn't supposed to be a function it's an IMAGE_DOS_HEADER that is used by the runtime loader.

I agree! But when I overload an address "no body knows" that it is a function it appears to be a normal address which could be form a variable as well.
Soo! When I use your patch this address will get called - proved by giving an address of a function.
But when I'm not using your patch, then this address won't be called.
This only happens to __ImageBase

[marsupial]

In D30709#1019309, @LepelTsmok wrote:

I think what you're trying to do just isn't unsupported at this time.

So it is supported?

Should have read: I think what you're trying to do just isn't supported at this time.

I will not always have the source code

Which seems to imply you also won't know the flags it was compiled with thus making it difficult to setup the target machine to match.

I agree! But when I overload an address "no body knows" that it is a function it appears to be a normal address which could be form a variable as well.

But you're using an object file that explicitly expects __ImageBase to be an IMAGE_DOS_HEADER header.
The code generated could be attempting to access data from offsets/sections that aren't mapped properly.

Using clang/llvm tools to generate the object file, would probably be a better start.
It might make more sense to post on the LLVM list as to exactly what you are trying to accomplish and why it must be done this way.

[• LepelTsmok]

Which seems to imply you also won't know the flags it was compiled with thus making it difficult to setup the target machine to match.

For the current case I know the source code, the compiler flags and everything. But their might be the case that I don't have the source code and only the object file.

But you're using an object file that explicitly expects __ImageBase to be an IMAGE_DOS_HEADER header.

Even if I set /__ImageBase to the original value, or the lowest section - it doesn't matter. When the crash occures the RIP register will always have the address I overloaded __ImageBase with.

Using clang/llvm tools to generate the object file, would probably be a better start.

I agree on that, but this can't help me with object files I don't have the code for.

It might make more sense to post on the LLVM list as to exactly what you are trying to accomplish and why it must be done this way.

I tried this already

llvm/trunk/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h

 class RuntimeDyldCOFFX86_64 : public RuntimeDyldCOFF {
 
 private:
   // When a module is loaded we save the SectionID of the unwind
   // sections in a table until we receive a request to register all
   // unregisteredEH frame sections with the memory manager.
   SmallVector<SID, 2> UnregisteredEHFrameSections;
   SmallVector<SID, 2> RegisteredEHFrameSections;
+  uint64_t ImageBase;
+
+  // Fake an __ImageBase pointer by returning the section with the lowest adress
+  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;
+  }
+
+  void write32BitOffset(uint8_t *Target, int64_t Addend, uint64_t Delta) {
+    uint64_t Result = Addend + Delta;
+    assert(Result <= UINT32_MAX && "Relocation overflow");
+    writeBytesUnaligned(Result, Target, 4);
+  }
 
 public:
   RuntimeDyldCOFFX86_64(RuntimeDyld::MemoryManager &MM,
                         JITSymbolResolver &Resolver)
-    : RuntimeDyldCOFF(MM, Resolver) {}
+    : RuntimeDyldCOFF(MM, Resolver), ImageBase(0) {}
 
-  unsigned getMaxStubSize() override {
-    return 6; // 2-byte jmp instruction + 32-bit relative address
-  }
+  unsigned getStubAlignment() override { return 1; }
+
+  // 2-byte jmp instruction + 32-bit relative address + 64-bit absolute jump
+  unsigned getMaxStubSize() override { return 14; }
 
   // The target location for the relocation is described by RE.SectionID and
   // RE.Offset.  RE.SectionID can be used to find the SectionEntry.  Each
   // SectionEntry has three members describing its location.
   // SectionEntry::Address is the address at which the section has been loaded
   // into memory in the current (host) process.  SectionEntry::LoadAddress is
   // the address that the section will have in the target process.
   // SectionEntry::ObjAddress is the address of the bits for this section in the
case COFF::IMAGE_REL_AMD64_REL32_5: {
       uint64_t Result = Value + RE.Addend;
       assert(((int64_t)Result <= INT32_MAX) && "Relocation overflow");
       assert(((int64_t)Result >= INT32_MIN) && "Relocation underflow");
       writeBytesUnaligned(Result, Target, 4);
       break;
     }
 
     case COFF::IMAGE_REL_AMD64_ADDR32NB: {
-      // Note ADDR32NB requires a well-established notion of
-      // image base. This address must be less than or equal
-      // to every section's load address, and all sections must be
-      // within a 32 bit offset from the base.
-      //
-      // For now we just set these to zero.
-      writeBytesUnaligned(0, Target, 4);
+      // ADDR32NB requires an offset less than 2GB from 'ImageBase'.
+      // The MemoryManager can make sure this is always true by forcing the
+      // memory layout to be: CodeSection < ReadOnlySection < ReadWriteSection.
+      const uint64_t ImageBase = getImageBase();
+      if (Value < ImageBase || ((Value - ImageBase) > UINT32_MAX)) {
+        llvm::errs() << "IMAGE_REL_AMD64_ADDR32NB relocation requires an"
+                     << "ordered section layout.\n";
+        write32BitOffset(Target, 0, 0);
+      } else {
+        write32BitOffset(Target, RE.Addend, Value - ImageBase);
+      }
       break;
     }
 
     case COFF::IMAGE_REL_AMD64_ADDR64: {
       writeBytesUnaligned(Value + RE.Addend, Target, 8);
       break;
     }
 
     default:
       llvm_unreachable("Relocation type not implemented yet!");
       break;
     }
   }
 
+  std::tuple<uint64_t, uint64_t, uint64_t>
+  generateRelocationStub(unsigned SectionID, StringRef TargetName,
+                         uint64_t Offset, uint64_t RelType, uint64_t Addend,
+                         StubMap &Stubs) {
+    uintptr_t StubOffset;
+    SectionEntry &Section = Sections[SectionID];
+
+    RelocationValueRef OriginalRelValueRef;
+    OriginalRelValueRef.SectionID = SectionID;
+    OriginalRelValueRef.Offset = Offset;
+    OriginalRelValueRef.Addend = Addend;
+    OriginalRelValueRef.SymbolName = TargetName.data();
+
+    auto Stub = Stubs.find(OriginalRelValueRef);
+    if (Stub == Stubs.end()) {
+      DEBUG(dbgs() << " Create a new stub function for " << TargetName.data()
+                   << "\n");
+
+      StubOffset = Section.getStubOffset();
+      Stubs[OriginalRelValueRef] = StubOffset;
+      createStubFunction(Section.getAddressWithOffset(StubOffset));
+      Section.advanceStubOffset(getMaxStubSize());
+    } else {
+      DEBUG(dbgs() << " Stub function found for " << TargetName.data() << "\n");
+      StubOffset = Stub->second;
+    }
+
+    // FIXME: If RelType == COFF::IMAGE_REL_AMD64_ADDR32NB we should be able
+    // to ignore the __ImageBase requirement and just forward to the stub
+    // directly as an offset of this section:
+    // write32BitOffset(Section.getAddressWithOffset(Offset), 0, StubOffset);
+    // .xdata exception handler's aren't having this though.
+
+    // Resolve original relocation to stub function.
+    const RelocationEntry RE(SectionID, Offset, RelType, Addend);
+    resolveRelocation(RE, Section.getLoadAddressWithOffset(StubOffset));
+
+    // adjust relocation info so resolution writes to the stub function
+    Addend = 0;
+    Offset = StubOffset + 6;
+    RelType = COFF::IMAGE_REL_AMD64_ADDR64;
+
+    return std::make_tuple(Offset, RelType, Addend);
+  }
+
   Expected<relocation_iterator>
   processRelocationRef(unsigned SectionID,
                        relocation_iterator RelI,
                        const ObjectFile &Obj,
                        ObjSectionToIDMap &ObjSectionToID,
                        StubMap &Stubs) override {
     // If possible, find the symbol referred to in the relocation,
     // and the section that contains it.
processRelocationRef(unsigned SectionID,
 
     // Determine the Addend used to adjust the relocation value.
     uint64_t RelType = RelI->getType();
     uint64_t Offset = RelI->getOffset();
     uint64_t Addend = 0;
     SectionEntry &Section = Sections[SectionID];
     uintptr_t ObjTarget = Section.getObjAddress() + Offset;
 
+    Expected<StringRef> TargetNameOrErr = Symbol->getName();
+    if (!TargetNameOrErr)
+      return TargetNameOrErr.takeError();
+    StringRef TargetName = *TargetNameOrErr;
+
     switch (RelType) {
 
     case COFF::IMAGE_REL_AMD64_REL32:
     case COFF::IMAGE_REL_AMD64_REL32_1:
     case COFF::IMAGE_REL_AMD64_REL32_2:
     case COFF::IMAGE_REL_AMD64_REL32_3:
     case COFF::IMAGE_REL_AMD64_REL32_4:
     case COFF::IMAGE_REL_AMD64_REL32_5:
     case COFF::IMAGE_REL_AMD64_ADDR32NB: {
       uint8_t *Displacement = (uint8_t *)ObjTarget;
       Addend = readBytesUnaligned(Displacement, 4);
+
+      if (IsExtern)
+        std::tie(Offset, RelType, Addend) = generateRelocationStub(
+          SectionID, TargetName, Offset, RelType, Addend, Stubs);
+
       break;
     }
 
     case COFF::IMAGE_REL_AMD64_ADDR64: {
       uint8_t *Displacement = (uint8_t *)ObjTarget;
       Addend = readBytesUnaligned(Displacement, 8);
       break;
     }
 
     default:
       break;
     }
 
-    Expected<StringRef> TargetNameOrErr = Symbol->getName();
-    if (!TargetNameOrErr)
-      return TargetNameOrErr.takeError();
-    StringRef TargetName = *TargetNameOrErr;
-
     DEBUG(dbgs() << "\t\tIn Section " << SectionID << " Offset " << Offset
                  << " RelType: " << RelType << " TargetName: " << TargetName
                  << " Addend " << Addend << "\n");
 
     if (IsExtern) {
       RelocationEntry RE(SectionID, Offset, RelType, Addend);
       addRelocationForSymbol(RE, TargetName);
     } else {
       bool IsCode = SecI->isText();
       unsigned TargetSectionID;
       if (auto TargetSectionIDOrErr =
           findOrEmitSection(Obj, *SecI, IsCode, ObjSectionToID))
         TargetSectionID = *TargetSectionIDOrErr;
       else
         return TargetSectionIDOrErr.takeError();
       uint64_t TargetOffset = getSymbolOffset(*Symbol);
       RelocationEntry RE(SectionID, Offset, RelType, TargetOffset + Addend);
       addRelocationForSection(RE, TargetSectionID);
     }
 
     return ++RelI;
   }
 
-  unsigned getStubAlignment() override { return 1; }
   void registerEHFrames() override {
     for (auto const &EHFrameSID : UnregisteredEHFrameSections) {
       uint8_t *EHFrameAddr = Sections[EHFrameSID].getAddress();
       uint64_t EHFrameLoadAddr = Sections[EHFrameSID].getLoadAddress();
       size_t EHFrameSize = Sections[EHFrameSID].getSize();
       MemMgr.registerEHFrames(EHFrameAddr, EHFrameLoadAddr, EHFrameSize);
       RegisteredEHFrameSections.push_back(EHFrameSID);
     }
     UnregisteredEHFrameSections.clear();
   }
+
   Error finalizeLoad(const ObjectFile &Obj,
                      ObjSectionToIDMap &SectionMap) override {
     // Look for and record the EH frame section IDs.
     for (const auto &SectionPair : SectionMap) {
       const SectionRef &Section = SectionPair.first;
       StringRef Name;
       if (auto EC = Section.getName(Name))
         return errorCodeToError(EC);
-      // Note unwind info is split across .pdata and .xdata, so this
-      // may not be sufficiently general for all users.
-      if (Name == ".xdata") {
+
+      // Note unwind info is stored in .pdata but often points to .xdata
+      // with an IMAGE_REL_AMD64_ADDR32NB relocation. Using a memory manager
+      // that keeps sections ordered in relation to __ImageBase is necessary.
+      if (Name == ".pdata")
         UnregisteredEHFrameSections.push_back(SectionPair.second);
-      }
-    }
     return Error::success();
   }
 };
 
 } // end namespace llvm
 
 #undef DEBUG_TYPE
 
 #endif

llvm/trunk/test/ExecutionEngine/RuntimeDyld/X86/COFF_x86_64_IMGREL.s

+# RUN: rm -rf %t && mkdir -p %t
+# RUN: llvm-mc -triple=x86_64-pc-win32 -filetype=obj -o %t/COFF_x86_64_IMGREL.o %s
+# RUN: llvm-rtdyld -triple=x86_64-pc-win32 -verify -check=%s %t/COFF_x86_64_IMGREL.o
+.text
+  .def   F;
+  .scl  2;
+  .type 32;
+  .endef
+  .globl  __constdata
+
+.section    .rdata, "dr", discard, __constdata
+    .align  8
+    __constdata:
+      .quad 0
+
+.text
+  .globl  F
+  .align  16, 0x90
+
+F:                                      # @F
+# rtdyld-check: decode_operand(inst1, 3) = section_addr(COFF_x86_64_IMGREL.o, .text)+0
+inst1:
+    mov %ebx, F@IMGREL
+# rtdyld-check: decode_operand(inst2, 3) = section_addr(COFF_x86_64_IMGREL.o, .rdata)+5
+inst2:
+    mov %ebx, (__constdata@imgrel+5)