Fix partial trimming of assemblies on referenced in metadata #91713
Conversation
runtime-extra-platforms are on the floor again after the rebrand to .NET 9 TFM PR. They're failing with an obscure: ``` Unhandled exception. System.InvalidOperationException: Unable to get member, please check input for IsNativeAot. at Microsoft.DotNet.XUnitExtensions.DiscovererHelpers.Evaluate(Type calleeType, String[] conditionMemberNames) + 0xb6 at Microsoft.DotNet.XUnitExtensions.DiscovererHelpers.<EvaluateArguments>d__13.MoveNext() + 0x209 ``` This is because `PlatformDetection.IsNativeAot` is not visible from reflection. Root caused this to: * `PlatformDetection` type is only referenced from `ConditionalFact` attribute. This attribute is not annotated with dataflow annotations, so the only thing we keep is the _metadata_ for the `PlatformDetection` type, since there's nothing else to keep (no methods, no MethodTable). * This is the _only_ reference to something from TestUtilities assembly. * Before the 9.0 rebrand PR, some nullable junk attributes were also generated into TestUtilities assembly. This basically helped us get _some runtime artifacts_ for _something_ in TestUtilities to be generated - when we were scanning attributes on PlatformDetection we'd say: oh this attribute needs a methodtable and a body for the constructor. This in turn triggered partial trimming logic to make _everything_ in TestUtilities reflection rooted. * After the 9.0 rebrand PR those attributes live in CoreLib, so nothing in TestUtilities gets a runtime artifact. Partial trimming logic is not looked at from TypeMetadata node. I'm fixing it by forcing a MethodTable whenever we have metadata for a `<Module>` type (this type is always generated). But also I'm indifferent about this fix. There are many similar corner cases where we'd miss that an assembly was used (e.g. declare a local of some type and then never do anything with it). I don't think we need to fix them. This could also be fixed by just rooting TestUtilities in our testing infra. I'm open to that too.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue Detailsruntime-extra-platforms are on the floor again after the rebrand to .NET 9 TFM PR. They're failing with an obscure: This is because Root caused this to:
I'm fixing it by forcing a MethodTable whenever we have metadata for a But also I'm indifferent about this fix. There are many similar corner cases where we'd miss that an assembly was used (e.g. declare a local of some type and then never do anything with it). I don't think we need to fix them. This could also be fixed by just rooting TestUtilities in our testing infra. I'm open to that too. Cc @dotnet/ilc-contrib
|
|
I've changed this to just root TestUtilities now that I had some time to think about this. It doesn't make sense to try to patch these. This code is trim unsafe and that's the actual problem. |
Yeah, this looks better to me. I had mixed feelings about the original fix. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
runtime-extra-platforms are on the floor again after the rebrand to .NET 9 TFM PR.
They're failing with an obscure:
This is because
PlatformDetection.IsNativeAotis not visible from reflection.Root caused this to:
PlatformDetectiontype is only referenced fromConditionalFactattribute. This attribute is not annotated with dataflow annotations, so the only thing we keep is the metadata for thePlatformDetectiontype, since there's nothing else to keep (no methods, no MethodTable).I'm fixing it by forcing a MethodTable whenever we have metadata for a<Module>type (this type is always generated).But also I'm indifferent about this fix. There are many similar corner cases where we'd miss that an assembly was used (e.g. declare a local of some type and then never do anything with it). I don't think we need to fix them. This could also be fixed by just rooting TestUtilities in our testing infra. I'm open to that too.I'm fixing it by just explicitly rooting TestUtilities.
Cc @dotnet/ilc-contrib