-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Linker to NativeAOT sync #71485
Linker to NativeAOT sync #71485
Conversation
ILC is multi-threaded and CompilerGeneratedState stores a global cache. So the cache has to be synchronized. This rewrites it to use LockFreeReaderHashtable.
@tlakollo could you please review the changes to CompilerGeneratedState specifically? In general the code should be the same as in linker, but it's now thread safe, so it required quite a few refactorings. |
I made some changes to the linker repo to make this port easier: dotnet/linker#2873 |
* Move GetDiagnosticCategory into shared code * Describe the intent around usage of DiagnosticContext in a comment * Minor fix in MessageOrigin GetHasCode * Add tests for patterns which NativeAOT had trouble with Linker changes made for dotnet/runtime#71485
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.
Still reviewing but leaving some comments for now
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedNames.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticContext.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
Outdated
Show resolved
Hide resolved
#if false | ||
: IComparable<MessageOrigin>, IEquatable<MessageOrigin> | ||
IComparable<MessageOrigin>, |
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 seems intentional, but not sure I follow why we need a statement that seems to never be executed because false is always false so it should always derive from IEquatable. Is it to show a difference between Linker and NativeAOT code?
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.
It was there before, so I kept it for now. I agree that we may eventually remove it, depends on how exact we want to be with the linker. Linker sorts all warnings before printing them out. I think it would make sense to do the same for ILC - even more so, since ILC is multi-threaded and thus more likely to produce warnings in random order. We should catch this as we port more tests over.
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 #if false
is equivalent to commenting out code. I used #if false
for larger blocks of code here. I tend to keep commented out code for stuff that consciously wasn't ported over for now.
We track determinism as "the output files should be the same". We haven't really bothered with ordering the warnings. Sometimes it's nice to get warnings from the tool even before the tool finishes. One can just "Ctrl-C" the tool and go fix the warning if it's relevant.
FYI: I will have to port the most recent changes from Sven over as well - with those it should fix the failures seen in CI. Ultimately we're hitting this bug: #85464. The linker doesn't crash due to changes from Sven, but NativeAOT currently crashes on it. |
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'm still looking through this, but this is how much I was able to look at today (I took a day off yesterday instead of today's public holiday).
if (genericArgumentType.IsTypeOf(ILLink.Shared.TypeSystemProxy.WellKnownType.System_Nullable_T)) | ||
{ | ||
var innerGenericArgument = genericArgumentType.Instantiation.Length == 1 ? genericArgumentType.Instantiation[0] : null; | ||
switch (innerGenericArgument) | ||
{ | ||
case GenericParameterDesc gp: | ||
return new NullableValueWithDynamicallyAccessedMembers(genericArgumentType, | ||
new GenericParameterValue(gp, GetGenericParameterAnnotation(gp))); | ||
|
||
case TypeDesc underlyingType: | ||
return new NullableSystemTypeValue(genericArgumentType, new SystemTypeValue(underlyingType)); | ||
} | ||
} | ||
// All values except for Nullable<T>, including Nullable<> (with no type arguments) | ||
return new SystemTypeValue(genericArgumentType); |
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 see that this is a pattern copied from ILLink so I don't expect it to be fixed here, but I'm not a big fan of code handling impossible states. They tend to hide bugs and make root causing bugs a lot harder.
Two things here:
- We "handle" situation when
Nullable<T>
doesn't have exactly one generic argument. - We "handle" when the instantiation argument is something bogus (null) - by unnecessarily using a non-exhaustive switch instead of a simple if/else.
I see this pattern in many places so I'm not going to comment on them, but it's something worth re-evaluating in the linker repo. Code that crashes when it hits invalid state is a lot easier to work with than code that papers over it and then crashes later (or worse - produces invalid outputs). We have zero test coverage for this invalid state handling.
A lot of the invalid state handling (not this one in particular) also seems to be forced by nullable reference types - I assume it wasn't easy to make something non-null so now we have code handling an impossible null scenario. Honesly, I would prefer hitting a NullReferenceException. Those exceptions are super easy to debug and often can be debugged from nothing but a stack trace. Complex code taking wrong turns is everything but easy to debug.
In the maxim "if I can't cover a branch in testing, the branch shouldn't exist". The null propagating operator ?.
is a branch too. We use them very liberally in this codebase. Many of those uses look as questionable as the question mark.
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 are two main reasons for some of this weird "handlings", but it doesn't excuse most of them:
- Cecil's reference->definition resolution returns null if the resolution fails (missing assembly dependency, wrong dependency, ...). So unlike in NativeAOT's type system, there are potentially lot of places where null can appear unfortunately.
- Historically we've seen quite a few cases where weird IL/metadata we never anticipated (like obfuscated code) crashed the linker. While I agree it's easy to diagnose the problem, there's typically absolutely no workaround for the user (other than disabling trimming which in Blazor/Xamarin scenarios is not really viable).
We are guilty of adding ?.
and similar in places where we couldn't immediately reason about the input being null or not. This is exaggerated by the fact that we turned on nullable reference types (which was a good thing as it found several issues), but Cecil is not annotated, so everything looks nullable (even if in really it might not be).
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedCallGraph.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedCallGraph.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
Outdated
Show resolved
Hide resolved
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.
Looks great. Just a couple opportunities to delete some lines of code.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs
Outdated
Show resolved
Hide resolved
This was needed to fix failures in libraries build with AOT. The underlying problem is not fully fixed: https://github.com/dotnet/linker/issues/2874 But with these changes it doesn't crash the compiler, it just leads to sometimes imprecise warnings.
The latest commit 53800b7 brings recent linker changes over. I suggest this is reviewed incrementally. I didn't have a chance to apply PR feedback from Michal yet. As is right now it passes smoke tests and libraries AOT build. |
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs
Show resolved
Hide resolved
@@ -20,6 +20,11 @@ | |||
<ProjectReference Include="..\ILCompiler.DependencyAnalysisFramework\ILCompiler.DependencyAnalysisFramework.csproj" /> | |||
<ProjectReference Include="..\ILCompiler.MetadataTransform\ILCompiler.MetadataTransform.csproj" /> | |||
<ProjectReference Include="..\ILCompiler.TypeSystem\ILCompiler.TypeSystem.csproj" /> | |||
|
|||
<PackageReference Include="StaticCs" Version="$(StaticCsVersion)"> |
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 think this will be a problem for source build.
It looks like this is just some analyzer with a custom attribute? I would be fine not running the analyzer in source build (or ever) and we can fake the custom attribute so that we don't have to make acrobatics in the shared files.
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.
@agocke - could you please comment on this? I basically just copied this from linker, I don't know the details behind it.
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.
Yup, it's an analyzer and a source file with attributes. I think we can condition it out in source build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Outdated
Show resolved
Hide resolved
For the
The test assemblies produce a lot of analysis warnings. We also don't care. |
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.
Thank you!
...at the cost of a small compat break. We return `int[]` instead of `SomeInt32Enum[]`. Fixes dotnet#72140. We can also delete intrinsic handling of `Enum.GetValues` in dataflow analysis but I don't want to conflict with Vitek's dotnet#71485.
…time into LinkerToAotSync2
If the user method has no intersting code in it, but it has a lambda (for example) which does have an interesting code which needs data flow - we now need to add the user method for data flow analysis (since the user method and all of its compiler generated methods will be analyzed together). This also adds a few smoke tests covering these scenarios.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
This brings changes from the linker repo to ilc roughly as of a week ago. This required some non-trivial changes to the code either already in ilc or the one coming from linker: