Fix assert in NativeAOT when handling data flow on compiler generated… #73085
Conversation
… code. The problem occurs when an entire type/assembly is preserved through explicit rooting (command line, rd.xml, ...). If such type contains a local function (for example) which is only called from a branch which is going to be removed by constant-prop/branch removal the internal tracking of compiler generated methods will see this local function as orphaned (not belonging to any user method). This leads to a case where we will try to run data flow on the local function - but that should never happen for compiler generated methods directly -> assert. The fix is (just like in the linker), to never run data flow on compiler generated methods directly - they should only run data flow as part of their respective user method. Fixes dotnet#73027
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
I don't have much context about the surrounding NativeAot code, but locally it LGTM. Thanks!
MichalStrehovsky
left a comment
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Another possible fix would be to not use ILProvider, but EcmaMethodIL.Create directly - that one sees untransformed IL before constprop. I could see that having both advantages and disadvantages.
| methodILDefinition = FlowAnnotations.ILProvider.GetMethodIL(userMethod); | ||
| } | ||
|
|
||
| // Data-flow (reflection scanning) for compiler-generated methods will happen as part of the |
There was a problem hiding this comment.
I only understood what's going on after re-reading the pull request description - could you add a sentence describing when this can happen (the lines before this are supposed to do this conversion to user code).
|
Merging because I'm running into this in #69108 and can't get a checked build of anything without this fix. We need to add checked test coverage to the CI matrix. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
… code.
The problem occurs when an entire type/assembly is preserved through explicit rooting (command line, rd.xml, ...). If such type contains a local function (for example) which is only called from a branch which is going to be removed by constant-prop/branch removal the internal tracking of compiler generated methods will see this local function as orphaned (not belonging to any user method). This leads to a case where we will try to run data flow on the local function - but that should never happen for compiler generated methods directly -> assert.
The fix is (just like in the linker), to never run data flow on compiler generated methods directly - they should only run data flow as part of their respective user method.
Fixes #73027