Fix pGeneratedNewStub determination #80128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
community-contribution
LakshanF
removed
the
community-contribution
@ChrisAhna Yes please. Your note regarding avoiding touching "old code" is correct, but in this case the unnecessary complexity is more of a cost than the risk of changing something that isolated and and simple to manually audit once applied. |
|
@AaronRobinsonMSFT, thanks for taking a look! I'll update the PR. |
|
@ChrisAhna Feel free to squash and merge when you are ready. |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3850655141 |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #80127.
See the issue, the especially the "Other Information" section, for details on the problem that is being fixed.
With this change in place, launching the "TryStructMarshal.exe 40" repro app in a loop runs indefinitely without ever failing (on the same NUMBER_OF_PROCESSORS=8 VM where "TryStructMarshal.exe 40" fails 10+% of the time without the change).
Beyond the changes in the PR, one other note is that the ILStubCreatorHelperHolder concept seems to be unnecessary. The operation of HolderLeave is identical to the operation of the ILStubCreatorHelper dtor. I believe that every path which calls HolderLeave will also imminently call the ILStubCreatorHelper dtor, meaning it should be safe to remove HolderLeave along with the ILStubCreatorHelperHolder concept as a whole (with the ILStubCreatorHelperHolder ctor calls just being rewritten as explicit calls to GetStubMethodDesc).
I didn't include this removal in the PR on the grounds that CreateInteropILStub is "old code" where it's often better to keep churn to a minimum. But I'd be happy to get rid of ILStubCreatorHelperHolder as well if there is interest in making a larger change here.