-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IR with double constant doesn't work with OrcJIT build by MSVC #43682
Comments
; ModuleID = 'module.bc' @PI = local_unnamed_addr constant double 3.140000e+00 ; Function Attrs: norecurse nounwind writeonly ; Function Attrs: norecurse nounwind readonly attributes #0 = { norecurse nounwind writeonly } |
.set @feat.00, 0 %bb.0:
getCachedValue: # @getCachedValue %bb.0:
PI:
|
Hi Alexander, Thanks for looking in to this. The proposed patch is not the right fix: It weakens responsibility checking and dynamically injects new definitions. I'm trying to wrap my head around what has gone wrong so that I can figure out the correct fix. From the original assertion it looks like the assertion that fired was in a call to JITDylib::resolve that included a symbol that wasn't defined in the JITDylib, but I'm not sure how that could have happened: We should have hit the corresponding assertion in MaterializationResponsibility::notifyResolved before we got to JITDylib::resolve. Would you be able to run this under a debugger and obtain a backtrace for the original assertion? If this analysis is correct: That the symbol has somehow made it into the responsibility set for the MaterializationResponsibility, but is missing from the JITDylib, then we need to track down where this correspondence broke. They should move in lock-step. |
.LCPI0_0: %bb.0:
.Lfunc_end0: %bb.0:
.Lfunc_end1:
|
Hi Lang, Thank you for the fast response. I had some debug session before I prepared my "hot fix" :) So what is happened in code in my understanding:
So in my opinion, 'calculation' depends on constant '__real@40091eb851eb851f' and we have to extend our 'Symbol' map. Best wishes, |
Another way just discard all unexpected symbols, but I don't enough familiar with OrcJIT architecture to say what way is proper. |
The patch which fixes the issue [updated]. |
Hi Lang, Is there any chance that this patch (or "true" fix) will be in LLVM 10? |
Hi Alexander, I'm just back from vacation. I'll try to get you an update on this today. -- Lang. |
Hi Alexander,
That all sounds right to me. I guess the code generator transformed this into a named constant (probably intended to be non-exported from the final symbol table). The thing that has me confused is the specific assertion that you saw: It is in JITDylib::resolve, but this is only called from MaterializationResponsibility::notifyResolved which contains what should have been an equivalent assertion: That the symbol appears in the SymbolFlags map of the MaterializationResponsibility. So what I want to know is how we managed to trigger the second assertion without triggering the first? Are you able to debug your LLVM build and tell me whether the loop in MaterializationResponsibility::notifyResolved is running? That mystery aside, I think the right answer here is to have RTDyldObjectLinkingLayer auto-claim responsibility for new symbols in the object. I've attached a patch to do that (auto-claim-obj-syms.patch). Would you be able to test it out for me? I do not have access to a Windows machine. -- Lang. |
We had a winter holidays. |
What version of LLVM is your patch for? |
Sorry for the confusion. This part of the patch fixes the issue #43681 (The root cause of the issue that the resolved symbol on COFF doesn't have flag JITSymbolFlags::Exported). |
Lang, I have adapted your patch for LLVM 9.0.0 (please take a look is it right) and have got the error at runtime: |
I think root cause of this error (Duplicate definition of symbol '__real@40091eb851eb851f') is here (return make_error(*KV.first);): Error JITDylib::defineMaterializing(const SymbolFlagsMap &SymbolFlags) {
}); Called from:
} if (!ExtraSymbolsToClaim.empty()) R.notifyResolved(Symbols); if (NotifyLoaded) return Error::success(); The second issue is here:
Despite the AutoClaimObjectSymbols flag is true extra symbol will be added to Symbols[InternedName]. |
Hi Alexander, Someone has reported a similar bug in http://bugs.#40074 . I have committed a (likely) fix for their issue in 84217ad. Would you be able to test and see if that commit fixes your issue too? -- Lang. |
Hi Lang, I have ported your patch from #40074 to LLVM 9.0.0 and tested it in the mine environment. But are you sure that it is a good idea to pass a parameter by value? One more question for you: is removing a module from OrcJIT will be available in LLVM 10? Best wishes, |
That's great news. :)
The JITDylib::defineMaterializing method makes modifications to the map based on what definitions are accepted and rejected, and returns these to MaterializationResponsibilty::defineMaterializing so that it can update the responsibility set. Since these modifications are not interesting to the client, it makes sense to pass by value: It is cheap, and if the client does not need their value any more then no copies are required.
Unfortunately support for removing modules will not make it in to LLVM 10. The best solution for now is to maintain a separate ExecutionSession for code that you want to throw away. I am working on removable module support, and hope to get it in to LLVM 11. |
mentioned in issue llvm/llvm-bugzilla-archive#44700 |
Extended Description
Precondition:
LLVM 9.0.0 built by MSVC 2019.
Steps to reproduce:
Expected result:
The function should be executed fine.
Actual result:
The OrcJIT failed on an assertion Core.cpp:867:
Assertion failed: I != Symbols.end() && "Symbol not found", file C:\Workspace\llvm-9.0.0.src\lib\ExecutionEngine\Orc\Core.cpp, line 867
Note:
I figured out that the missed symbol is __real@40091eb851eb851f. It appears after IR code is compiled by llc to assembler.
This issue is not observed on Linux and MinGW builds on Windows.
The text was updated successfully, but these errors were encountered: