This is an archive of the discontinued LLVM Phabricator instance.

Handle IMAGE_REL_AMD64_ADDR32NB in RuntimeDyldCOFF
ClosedPublic

Authored by marsupial on Mar 7 2017, 11:19 AM.

Details

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.

Diff Detail

Event Timeline

marsupial created this revision.Mar 7 2017, 11:19 AM
martell added a subscriber: martell.May 1 2017, 8:07 AM

@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 requested changes to this revision.Jun 3 2017, 3:04 PM
compnerd added inline comments.
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

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

Why the extra scope?

145

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

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

Remove the newline.

279

Typo relation -> relation.

This revision now requires changes to proceed.Jun 3 2017, 3:04 PM
marsupial updated this revision to Diff 101421.Jun 5 2017, 9:44 AM
marsupial edited edge metadata.
marsupial marked 2 inline comments as done.Jun 5 2017, 9:53 AM
marsupial added inline comments.
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

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

Scoping is for the two temporary objects:

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

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

marsupial marked an inline comment as done.Jun 19 2017, 8:19 AM

Ping.

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.

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
36–43

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

114

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

131

Space after the std::tuple.

138

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

Stub is probably a better than name than Itr.

154–155

The else should be coddled.

167

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

284–286

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

marsupial added inline comments.Dec 11 2017, 9:53 AM
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

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.

167

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.

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 added a subscriber: LepelTsmok.EditedFeb 6 2018, 5:42 AM

//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 marked 12 inline comments as done.Feb 15 2018, 12:10 PM

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

compnerd accepted this revision.Feb 17 2018, 1:00 PM
compnerd added inline comments.
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

This really does belong in RuntimeDyldCOFF rather than here.

48

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?

This revision is now accepted and ready to land.Feb 17 2018, 1:00 PM

Changed name of function to write32BitOffset.

marsupial marked an inline comment as done.Feb 18 2018, 12:47 PM
marsupial added inline comments.
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

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.

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

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
36–43

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

This revision was automatically updated to reflect the committed changes.

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

LepelTsmok added a comment.EditedFeb 22 2018, 5:52 AM

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

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

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!

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

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.

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

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.

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