Skip to content
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

Support RunClassConstructor in the linker #1121

Merged
merged 6 commits into from
Apr 22, 2020

Conversation

tlakollo
Copy link
Contributor

This fixes #970

-Add Support for RuntimeHelpers.RunClassConstructor
-Add test for RunClassConstructor
-Change MarkStaticConstructor to internal in order to be able to call it
from ReflectionMethodBodyScanner

…_RunClassConstructor

-Add Support for RuntimeHelpers.RunClassConstructor
-Add test for RunClassConstructor
-Change MarkStaticConstructor to internal in order to be able to call it
from ReflectionMethodBodyScanner
@tlakollo tlakollo requested a review from vitek-karas April 17, 2020 20:53
@tlakollo tlakollo requested a review from marek-safar as a code owner April 17, 2020 20:53
@tlakollo tlakollo self-assigned this Apr 17, 2020
var parameters = calledMethod.Parameters;
foreach (var TypeHandleValue in methodParams [0].UniqueValues ()) {
if (TypeHandleValue is RuntimeTypeHandleValue runtimeTypeHandleValue) {
_markStep.MarkStaticConstructor (runtimeTypeHandleValue.TypeRepresented, new DependencyInfo (DependencyKind.AccessedViaReflection, reflectionContext.MethodCalling));
Copy link
Contributor

@marek-safar marek-safar Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be in DependencyKind.AccessedViaReflection category?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create a new one I guess. But it's not that different from something like type.GetConstructor().Invoke(). I do think it's still reflection, even if the method/type is not in the reflection namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we made a new category for this, it would set a precedent and we'll need yet another category for e.g. FormatterServices.GetUninitializedObject - these are all reflection-like APIs...

Tlakollo added 2 commits April 20, 2020 11:50
for RunClassConstructor
-Indent default for calledMethod.Name switch
-IntrinsicId.Type_get_TypeHandle needs to handle MergePointValues
-Handle NullValue inside IntrinsicId.Type_get_TypeHandle for
MergePointValues
-Change RequireDynamicallyAccessedMembers call si
MergePointVallyAccessedMemberKinds.DefaultConstructor does not mantain
static .cctor and implement a way to handle Null values and unrecognized
patterns
-Add more information into test, Recognized and Unrecognized patterns
-Differentiate test that use a Type from the ones that use a
RuntimeTypeHandle
-Add test of unknown value in conditional
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the couple comments this looks good.

-Edit error message to give more information to the user
@marek-safar marek-safar reopened this Apr 21, 2020
var parameters = calledMethod.Parameters;
foreach (var TypeHandleValue in methodParams [0].UniqueValues ()) {
if (TypeHandleValue is RuntimeTypeHandleValue runtimeTypeHandleValue) {
_markStep.MarkStaticConstructor (runtimeTypeHandleValue.TypeRepresented, new DependencyInfo (DependencyKind.AccessedViaReflection, reflectionContext.MethodCalling));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we made a new category for this, it would set a precedent and we'll need yet another category for e.g. FormatterServices.GetUninitializedObject - these are all reflection-like APIs...

} else if (typeHandleValue == NullValue.Instance)
reflectionContext.RecordHandledPattern ();
else {
reflectionContext.RecordUnrecognizedPattern ($"A {GetValueDescriptionForErrorMessage (typeHandleValue)} " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit/not really necessary: If we really wanted - the MemberKinds.Constructors dataflow annotation keeps static constructors because they can be accessed with typeof(Foo).GetConstructors(BindingFlags.NonPublic | BindingFlags.Static)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. For now I would leave it as is. Supporting the annotation would mean to flow it through the get_TypeHandle call as well. Until we hit a use case I'm perfectly fine supporting just the simple constant cases.

@tlakollo tlakollo merged commit 17b3c01 into dotnet:master Apr 22, 2020
@tlakollo tlakollo deleted the RunClassConstructor branch April 22, 2020 07:34
tkapin pushed a commit to tkapin/runtime that referenced this pull request Jan 31, 2023
Support RunClassConstructor in the linker

Commit migrated from dotnet/linker@17b3c01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pattern recognition for calls to RuntimeHelpers.RunClassConstructor
4 participants