ILLink: Remove ScopeStack from MarkStep #102282
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
This reverts commit 3557326.
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
dotnet-policy-service
bot
added
the
linkable-framework
jtschuster
commented
May 15, 2024
Comment on lines
-1813
to
-1817
| if (origin.Provider != ScopeStack.CurrentScope.Origin.Provider) { | ||
| Debug.Assert (dependencyKind == DependencyKind.DynamicallyAccessedMemberOnType || | ||
| (origin.Provider is MethodDefinition originMethod && CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (originMethod))); | ||
| } | ||
|
|
There was a problem hiding this comment.
This assertion was to make sure that we always passed ScopeStack.CurrentScope.Origin unless the call to MarkField originated from the ReflectionMarker. It always used the parameter origin, so the behavior is the same and this check isn't necessary.
jtschuster
commented
May 15, 2024
Comment on lines
-3061
to
-3070
| // There are only two ways to get there such that the origin isn't the same as the top of the scopestack. | ||
| // - For DAM on type, the current scope is the caller of GetType, while the origin is the type itself. | ||
| // - For warnings produced inside compiler-generated code, the current scope is the user code that | ||
| // owns the compiler-generated code, while the origin is the compiler-generated code. | ||
| // In either case any warnings produced here should use the origin instead of the scopestack. | ||
| if (origin.Provider != ScopeStack.CurrentScope.Origin.Provider) { | ||
| Debug.Assert (dependencyKind == DependencyKind.DynamicallyAccessedMemberOnType || | ||
| (origin.Provider is MethodDefinition originMethod && CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (originMethod))); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is similar to above with ProcessAnalysisAnnotationsForField. Since we no longer use ScopeStack, this can be removed.
jtschuster
removed
the
linkable-framework
dotnet-policy-service
bot
added
the
linkable-framework
|
Thanks a lot - I always hated this part of the mark step, but was never brave enough to tackle it :-). I remember that when I introduced this even then we didn't like it very much, but it worked... sooo. |
Ruihan-Yin
pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
Removes the ScopeStack from the MarkStep to make the MessageOrigin flow clearly traceable. This should be very helpful as we transition to the dependency analysis framework. The changes were fairly mechanical, each place that pushed a scope onto the stack, a new MessageOrigin was created, and all places that used ScopeStack.CurrentScope added a new origin parameter, and the parameter bubbled up to the ProcessX methods. Passing MessageOrigin by value was slightly faster than as an in parameter (and overall this is slightly faster than main), so I made MessageOrigin.ILOffset a non-nullable int to make the struct a bit smaller. I ran the aspnetcore benchmarks trim step and didn't run into issues, so the stack should be large enough to handle it.
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.
Removes the
ScopeStackfrom theMarkStepto make theMessageOriginflow clearly traceable. This should be very helpful as we transition to the dependency analysis framework.The changes were fairly mechanical, each place that pushed a scope onto the stack, a new
MessageOriginwas created, and all places that usedScopeStack.CurrentScopeadded a neworiginparameter, and the parameter bubbled up to theProcessXmethods.Passing
MessageOriginby value was slightly faster than as aninparameter (and overall this is slightly faster than main), so I madeMessageOrigin.ILOffseta non-nullableintto make the struct a bit smaller. I ran the aspnetcore benchmarks trim step and didn't run into issues, so the stack should be large enough to handle it.