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.
Details
Diff Detail
Event Timeline
@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.
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. |
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 |
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. |
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. |
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>
//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
Added test case for IMAGE_REL_AMD64_ADDR32NB with JIT and requested formatting changes.
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h | ||
---|---|---|
36–43 | Not sure what you mean... |
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. |
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.
- 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!
- 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
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
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__.