Remove base type rooting for types in rooted assemblies #92864
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsWhen rooting entire assemblies NativeAOT also roots all base types of all the types in such assembly. Regardless of which assembly the base type comes from. Historically this was necessary to make certain combinations of generic instantiations to work, but that's no longer the case. The compiler has better mechanism of tracking necessary things for generics now. This rooting behavior brings in more code than necessary. This is specifically problematic when using the assembly rooting as a mechanism to test library trim/AOT compatibility. If the tested library depends on another library which has some incompatible code in it, but it's not used, ideally the compiler should remove the unused code and thus not see the incompatibilities. Rooting entire base types sometimes breaks that behavior and produces unnecessary warnings. The product change is really just "don't root the base type", all of the rest of the change is tests. Modified existing test to validate more things around rooting behavior across assemblies. And then infrastructure changes to make it possible to use this test from NativeAOT. This brings the behavior of NativeAOT and trimmer much closer with regard to assembly rooting. Fixes #92271.
|
|
I changed it to only "Stop" the old behavior for rooted assemblies. rd.xml rooting will remain as before. Updated the trimming tests accordingly. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
When rooting entire assemblies NativeAOT also roots all base types of all the types in such assembly. Regardless of which assembly the base type comes from. Historically this was necessary to make certain combinations of generic instantiations to work, but that's no longer the case. The compiler has better mechanism of tracking necessary things for generics now. This rooting behavior brings in more code than necessary. This is specifically problematic when using the assembly rooting as a mechanism to test library trim/AOT compatibility. If the tested library depends on another library which has some incompatible code in it, but it's not used, ideally the compiler should remove the unused code and thus not see the incompatibilities. Rooting entire base types sometimes breaks that behavior and produces unnecessary warnings. The product change is really just "don't root the base type", all of the rest of the change is tests. Modified existing test to validate more things around rooting behavior across assemblies. And then infrastructure changes to make it possible to use this test from NativeAOT. This brings the behavior of NativeAOT and trimmer much closer with regard to assembly rooting.
Rooting from rd.xml is kept as before (rooting a type will also root its base types). Given that rd.xml is already very problematic and tricky and people rely on all kinds of "accidental" behavior, it's not worth the break - there's not much complexity in keeping this behavior there. Update trimming tests which started to fail after the infra updates. Mostly disable the tests for NativeAOT as they don't make sense on AOT.
vitek-karas
force-pushed
the
NativeAOTRooting
branch
from
8d31d45
to
b7ebb3b
Compare
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@MichalStrehovsky could you please take another look - specifically at the test changes. |
| @@ -171,15 +171,15 @@ protected override void Dispose(bool disposing) | |||
| public override void ImportParameters(DSAParameters parameters) => _dsa.ImportParameters(parameters); | |||
| public override bool VerifySignature(byte[] rgbHash, byte[] rgbSignature) => _dsa.VerifySignature(rgbHash, rgbSignature); | |||
| protected override byte[] HashData(Stream data, HashAlgorithmName hashAlgorithm) => | |||
| (byte[])_dsa.GetType().GetMethod( | |||
| (byte[])typeof(DSA).GetMethod( | |||
There was a problem hiding this comment.
There are multiple implementations of DSA/ECDSA so this is technically getting a different method, but since it's virtual it probably doesn't matter. Just wanted to point out for double checking.
There was a problem hiding this comment.
Thanks - I was aware of that, but as you stated, the method is declared as virtual on the DSA class, so unless some derived class does new this will work (and new would be really weird for a protected virtual method). If this was in product I would be much more worried, but for tests I think this is perfectly OK.
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6485720697 |
ghost
locked as resolved and limited conversation to collaborators
When rooting entire assemblies NativeAOT also roots all base types of all the types in such assembly. Regardless of which assembly the base type comes from. Historically this was necessary to make certain combinations of generic instantiations to work, but that's no longer the case. The compiler has better mechanism of tracking necessary things for generics now.
This rooting behavior brings in more code than necessary. This is specifically problematic when using the assembly rooting as a mechanism to test library trim/AOT compatibility. If the tested library depends on another library which has some incompatible code in it, but it's not used, ideally the compiler should remove the unused code and thus not see the incompatibilities. Rooting entire base types sometimes breaks that behavior and produces unnecessary warnings.
The product change is really just "don't root the base type", all of the rest of the change is tests. Modified existing test to validate more things around rooting behavior across assemblies. And then infrastructure changes to make it possible to use this test from NativeAOT.
This brings the behavior of NativeAOT and trimmer much closer with regard to assembly rooting.
Fixes #92271.