Fix ILLink/ILC hang when tracking too many hoisted local values #95041
Conversation
sbomer
requested review from
marek-safar and
MichalStrehovsky
as code owners
dotnet-issue-labeler
bot
added
the
needs-area-label
ghost
assigned 
|
|
||
| // ValueSet<TValue> is not enumerable. This helper translates ValueSet<SingleValue>.Unknown | ||
| // into a ValueSet<SingleValue> whose sole element is UnknownValue.Instance. | ||
| internal static IEnumerable<SingleValue> AsEnumerable (this MultiValue multiValue) |
There was a problem hiding this comment.
I must admit I'm not super excited about the need for this... but I also don't know about a better way - other than teaching ValueSet about the "error" value for all value sets (which is not ideal either).
There was a problem hiding this comment.
The other disadvantage of this approach is that each enumeration will now allocate, actually twice, once to box the enumerable and then to create the enumerator. If value set implement enumerable, it can rely on the compiler optimization to avoid these instead.
There was a problem hiding this comment.
One other approach might be this (not pretty, but might work):
Make MultiValue a wrapper (since struct can't inherit) - so declare MultiValue as a real type, and make it wrap the ValueSet. And them make it implement IEnumerable directly.
Not sure if there are places though where we heavily rely on the ability to operate on ValueSets directly. On the other hand the conversion could be implicit (since wrapping doesn't induce allocation), so it might be possible handle this nicely almost everywhere.
There was a problem hiding this comment.
Or (just idea):
- Change ValueSetLattice to a class (do we need this to be a struct, does it make much of a difference?)
- There some weird things about the lattice as is now
- It's a struct, but we use it through an interface -> boxing it anyway
- It's a struct, so can't be derived, thus the implementation of all of its methods is effectively
static. - For other lattices it's benefitial to be instance based (we need it in some cases), but for the ValueSetLattice this seems unnecessary, so having it provide the Top/Bottom as statics should work (alongside the interface implementations). Or maybe it would just provide the singleton value (it could be a singleton, we don't need to allocate more than one since they're all the same).
- Have ValueSet be a generic over TLattice
- Make the "Error" value a static on the lattice - or more precisely, make the ValueSetLattice define both Top and Bottom (only the ValueSetLattice for now, we would not need it on all lattices).
This would make the design overall slightly cleaner. For example now we rely on the implicit rule that the default value of a ValueSet is the Top value of the lattice.
There was a problem hiding this comment.
I changed this to return the Enumerable struct directly to avoid boxing. Does that address the concern or would you prefer an approach that avoids the AsEnumerable callsites?
Personally I don't mind since it makes it clear what's happening at each callsite - but if we want to avoid that then I like the suggestion to make MultiValue a wrapper.
About the second idea:
I think ValueSetLattice could be made a class pretty easily - though I think we currently manage to avoid boxing the struct in some cases by using generic constraints. I didn't think we had a case where we needed the lattice type to be instance based - I thought they were all effectively static, and the only reason we use instances is to implement the interface methods.
However, I'm reluctant to add the "Error" value to the lattice since there's no logic (yet) where the lattice-based dataflow directly needs a "Bottom" value. It also feels a bit weird to make ValueSet generic over TLattice since the other lattice types are the other way around. Also, if we did define "Bottom" on the lattice I think it should be the ValueSet<TValue>.Unknown version that's not enumerable, so I think we'd still need another solution to avoid the AsEnumerable/boxing conversion.
src/tools/illink/src/ILLink.Shared/TrimAnalysis/ValueExtensions.cs
Outdated
Show resolved
Hide resolved
marek-safar
added
the
area-Tools-ILLink
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue Details#87634 introduced a limit on the number of values tracked in dataflow analysis, but this approach was incorrect because resetting the set of tracked values was effectively moving up the dataflow lattice, breaking the invariant and causing potential hangs. The hang was observed in #94831, where (as found by @vitek-karas) the hoisted local The fix is to instead introduce a special value In this testcase the Fixes #94831.
|
- Use readonly - Avoid some allocations - Use sentinel class instead of enum
src/tools/illink/src/ILLink.Shared/TrimAnalysis/ValueExtensions.cs
Outdated
Show resolved
Hide resolved
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7011402325 |
|
@sbomer backporting to release/8.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix ILLink/ILC hang when tracking too many hoisted local values
Using index info to reconstruct a base tree...
M src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
M src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
A src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureContextLattice.cs
M src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs
M src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
A src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs
M src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisAssignmentPattern.cs
M src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
M src/tools/illink/src/ILLink.Shared/DataFlow/ValueSet.cs
M src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
M src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs
M src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
M src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExponentialDataFlow.cs
Falling back to patching base and 3-way merge...
Auto-merging src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExponentialDataFlow.cs
CONFLICT (content): Merge conflict in src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExponentialDataFlow.cs
Auto-merging src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
Auto-merging src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs
Auto-merging src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
Auto-merging src/tools/illink/src/ILLink.Shared/DataFlow/ValueSet.cs
CONFLICT (content): Merge conflict in src/tools/illink/src/ILLink.Shared/DataFlow/ValueSet.cs
Auto-merging src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
CONFLICT (content): Merge conflict in src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
Auto-merging src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisAssignmentPattern.cs
CONFLICT (content): Merge conflict in src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisAssignmentPattern.cs
CONFLICT (modify/delete): src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs deleted in HEAD and modified in Fix ILLink/ILC hang when tracking too many hoisted local values. Version Fix ILLink/ILC hang when tracking too many hoisted local values of src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs left in tree.
Auto-merging src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
CONFLICT (content): Merge conflict in src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
Auto-merging src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs
CONFLICT (content): Merge conflict in src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs
CONFLICT (modify/delete): src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureContextLattice.cs deleted in HEAD and modified in Fix ILLink/ILC hang when tracking too many hoisted local values. Version Fix ILLink/ILC hang when tracking too many hoisted local values of src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureContextLattice.cs left in tree.
Auto-merging src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
Auto-merging src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix ILLink/ILC hang when tracking too many hoisted local values
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@sbomer an error occurred while backporting to release/8.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
…et#95041) dotnet#87634 introduced a limit on the number of values tracked in dataflow analysis, but this approach was incorrect because resetting the set of tracked values was effectively moving up the dataflow lattice, breaking the invariant and causing potential hangs. The hang was observed in dotnet#94831, where (as found by @vitek-karas) the hoisted local `state` field of an async method with many await points was getting a large number of tracked values, hitting the limit, being reset to "empty", but then growing again, causing the analysis not to converge. The fix is to instead introduce a special value `ValueSet<TValue>.Unknown` that is used for this case. It acts like "bottom" in the lattice, so that it destroys any other inputs on meet ("bottom" meet anything else is "bottom"). In this testcase the `state` field isn't involved in dataflow warnings, so this actually allows the method to be analyzed correctly, producing the expected warning for the `Type` local that flows across all of the await points. Fixes dotnet#94831
#87634 introduced a limit on the number of values tracked in dataflow analysis, but this approach was incorrect because resetting the set of tracked values was effectively moving up the dataflow lattice, breaking the invariant and causing potential hangs.
The hang was observed in #94831, where (as found by @vitek-karas) the hoisted local
statefield of an async method with many await points was getting a large number of tracked values, hitting the limit, being reset to "empty", but then growing again, causing the analysis not to converge.The fix is to instead introduce a special value
ValueSet<TValue>.Unknownthat is used for this case. It acts like "bottom" in the lattice, so that it destroys any other inputs on meet ("bottom" meet anything else is "bottom").In this testcase the
statefield isn't involved in dataflow warnings, so this actually allows the method to be analyzed correctly, producing the expected warning for theTypelocal that flows across all of the await points.Fixes #94831.