Merged
Conversation
ghost
added
the
AustinWise
force-pushed
the
austin/FailFastQCall
branch
from
c535a1c to
aed8fd4
Compare
teo-tsirpanis
added
the
community-contribution
Member
@dotnet/dotnet-diag Could you please comment on impact of this change on Watson? |
Contributor
Author
|
I checked the reflection case and confirmed that the calculated return address is in the same location as before this change. |
Member
@leculver @mikem8361 - I think you guys are in the best position to evaluate. |
Contributor
|
It look like it should have any impact on Watson to me. |
mikem8361
approved these changes
Feb 28, 2024
jkotas
reviewed
Feb 28, 2024
src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Feb 28, 2024
src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <[email protected]>
AustinWise
added a commit
to AustinWise/runtime
that referenced
this pull request
Mar 16, 2024
This has a similar structure to FailFast as converted in dotnet#98908. Contributes to dotnet#95695
jkotas
added a commit
that referenced
this pull request
Mar 17, 2024
* Convert GetCurrentMethod to QCALL This has a similar structure to FailFast as converted in #98908. Contributes to #95695 * Simplify code for QCALL Co-authored-by: Jan Kotas <[email protected]> --------- Co-authored-by: Jan Kotas <[email protected]>
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
I confirmed that the instruction pointer found by stack walking matched the IP found by the FCALL version (pointing to the return address of the function that called FailFast). I do not know if that is sufficient to preserve the "Watson bucketization" that the many comments around this code mention.
TODO:
retAdressis the same after this change for direct calls toFailFast. It is the return address in the calling function.CallDescrWorkerInternalatCallDescrWorkerInternalReturnAddress. Maybe that is not the best address for bucketization, but it's the same address as before.GenericFailFastdoes a GCPROTECT and the QCALLS don't do anything that triggers a GC before callingGenericFailFast, so are they needed?Contributes to #95695