-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Enable inlining P/Invokes into try blocks with no catch or filter clauses #73032
Enable inlining P/Invokes into try blocks with no catch or filter clauses #73032
Conversation
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
|
||
if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pFrame)->m_pCallerReturnAddress)) | ||
TADDR returnAddress = ((InlinedCallFrame*)pFrame)->m_pCallerReturnAddress; | ||
#ifdef USE_PER_FRAME_PINVOKE_INIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is corinfo.h guaranteed to be included here to pickup this #define
?
corinfo.h
is JIT/EE interface so it does not need to be included globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one of those things we should set in the CMake files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've explicitly included corinfo.h
. Given that this behavior is effectively part of the JIT/EE contract (as one changing behavior will generally require corresponding work in the other to avoid crashes), I think corinfo.h
is a good place for it to live.
…g c++ source files.
…f a catch handler is present.
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Failures are #73247 This is ready for another round of review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JIT changes LGTM, just one small nit.
Failures are #73247 and the timeouts that we've been seeing. |
…lter clauses (dotnet#73032)" This reverts commit 4c07f3d. We believe it is causing recent CI failures. See dotnet#73247
…ch or filter clauses (dotnet#73032)" (dotnet#73551)" This reverts commit 2a14280.
Contributes to #70109