SPMI: Capture EE API exceptions in getNewHelper, and simplify the API shape a bit #89852
Conversation
We have various tests where getNewHelper throws an EE exception, so capture these and rethrow them as part of SPMI. It would be nice with a more general approach to this (conceptually all EE APIs could throw exceptions), but this at least handles it for our own libraries. Contributes to dotnet#47540 and dotnet#47546.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWe have various tests where getNewHelper throws an EE exception, so It would be nice with a more general approach to this (conceptually all Contributes to #47540 and #47546. Also clean up the API shape of
|
src/coreclr/vm/jitinterface.cpp
Outdated
| @@ -5772,7 +5772,7 @@ bool __stdcall TrackAllocationsEnabled() | |||
| } | |||
|
|
|||
| /***********************************************************************/ | |||
| CorInfoHelpFunc CEEInfo::getNewHelper(CORINFO_RESOLVED_TOKEN * pResolvedToken, CORINFO_METHOD_HANDLE callerHandle, bool * pHasSideEffects) | |||
| CorInfoHelpFunc CEEInfo::getNewHelper(CORINFO_CLASS_HANDLE classHandle, CORINFO_METHOD_HANDLE callerHandle, bool* pHasSideEffects) | |||
There was a problem hiding this comment.
Can callerHandle be deleted? It looks unused.
There was a problem hiding this comment.
It's used for diagnostics by crossgen2 – I initially actually was deleting it, until I ran into that use.
There was a problem hiding this comment.
We can replace the use by methodBeingCompiled -- it will have slightly different behavior when inlining, but maybe that's ok?
There was a problem hiding this comment.
Actually it shouldn't have any visible behavior difference since if crossgen2 throws the exception during inlining it will just result in the inlining attempt being aborted, and the exception won't be surfaced to the user. So let me make this change.
There was a problem hiding this comment.
We can replace the use by methodBeingCompiled -- it will have slightly different behavior when inlining, but maybe that's ok?
I do not think it will have different observable behavior. We catch and ignore exceptions during inlining, so the only place where this error can show up is when we fail during compilation of the top level method. When we fail during compilation of the top-level method, we are going to print its name in front of the error message. It is not necessary to duplicate the name of the method that failed to compile in the error message.
|
cc @dotnet/jit-contrib PTAL @BruceForstall I'll wait with merging until the weekend. |
There was a problem hiding this comment.
It would be nice with a more general approach to this (conceptually all
EE APIs could throw exceptions), but this at least handles it for our
own tests, and these situations are expected to be rare.
Seems like we should stop throwing exceptions across the JIT-EE API and instead use error codes. There are very few cases where we see exceptions today.
| CORINFO_CLASS_HANDLE classHandle, | ||
| bool* pHasSideEffects |
There was a problem hiding this comment.
Can you update the above documentation comment? It refers to "newCls", which doesn't exist (and didn't before, either), and doesn't describe pHasSideEffects.
That sounds hard since these exceptions are meant to be surfaced to the user and not handled by the JIT, so the JIT would need to throw the proper exception itself based on the error returned by the EE API (with the same amount of details, presumably). Plus it would have additional throughput cost on calls to many hot JIT-EE APIs, like IMO this is just an SPMI issue -- if we were auto generating (the majority of) the recording/replay layer of SPMI then it would be much easier to make a broad change like this. Our representation of exceptions in the collections is also unfortunate, requiring changes in the "value" struct that adds size overhead in the normal case and requires a JIT-EE GUID update in PRs like this one. An alternative to make it more pay-for-play would be to have separate packet IDs (or tag it in the upper bit) for the exceptions, and check both tables before reporting "missing". |
ghost
locked as resolved and limited conversation to collaborators
We have various tests where getNewHelper throws an EE exception, so
capture these and rethrow them as part of SPMI.
It would be nice with a more general approach to this (conceptually all
EE APIs could throw exceptions), but this at least handles it for our
own tests, and these situations are expected to be rare.
Contributes to #47540 and #47546.
Also clean up the API shape of
getNewHelperslightly, since this requiresa JIT-EE GUID change anyway; the only used member of the token is the class,
so pass a class handle instead.