-
-
Notifications
You must be signed in to change notification settings - Fork 984
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
Improve support for NativeAOT #1960
Conversation
…ing runtimes that are not longer supported (nobody should be using this combination)
…T/NativeAOT naming)
…rs(DynamicallyAccessedMemberTypes.All)]
@@ -11,6 +12,6 @@ public class GenericTypeArgumentsAttribute : Attribute | |||
// CLS-Compliant Code requires a constructor without an array in the argument list | |||
[PublicAPI] public GenericTypeArgumentsAttribute() => GenericTypeArguments = new Type[0]; | |||
|
|||
public GenericTypeArgumentsAttribute(params Type[] genericTypeArguments) => GenericTypeArguments = genericTypeArguments; | |||
public GenericTypeArgumentsAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] params Type[] genericTypeArguments) => GenericTypeArguments = genericTypeArguments; |
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.
Unfortunately placing the DAMAttribute on an array is a no-op. Arrays are difficult to model in a static analysis.
This should be generating a warning along the lines of Trim analysis warning IL2098: GenericTypeArgumentsAttribute..ctor(Type[]): Parameter '#1' of method 'GenericTypeArgumentsAttribute..ctor(Type[])' has 'DynamicallyAccessedMembersAttribute', but that attribute can only be applied to parameters of type 'System.Type' or 'System.String'.
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.
this actually broke building dotnet/performance
with dotnet/runtime
. When building a wasm project, I get - https://gist.github.com/radical/b75b5e4063d97be0adac729b611990ea . Can we revert this change? @adamsitnik
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.
If I remove that attribute from the param, then it works fine.
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.
To be clear, this breaks when I update bdn
reference in dotnet/performance
to 0.13.1.1740
.
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.
The attribute is a no-op - it should be removed because even in the absence of the illink bug, it generates a warning. NativeAOT doesn't have the linker bug and that's why it works there. I filed https://github.com/dotnet/linker/issues/2714 on the linker bug - that one should also be fixed independently of this removal.
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.
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 can build with that change 👍
With the following 3 recent fixes:
It required very little work to improve the NativeAOT support. Main changes:
--runtimes
no longer supportscorert70
. The proper name isnativeaot70
(case and dots are ignored, so you can useNativeAOT7.0
as well)CoreRtToolchainBuilder
has been upgraded to .NET 7 feedCoreRtToolchainBuilder
has been upgraded to7.0.0-*
--coreRtVersion
to--ilCompilerVersion
as it specifies the ILCompiler version. Sample command:Demo:
In the meantime I am working on getting all dotnet/performance microbenchmarks running with NativeAOT
cc @jkotas @MichalStrehovsky @hez2010 @Beau-Gosse-dev