Skip to content
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

Enable IlcTrimMetadata by default #70201

Merged
merged 13 commits into from
Jun 15, 2022
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<IlcArg Condition="$(IlcDumpIL) == 'true'" Include="--ildump:$(NativeIntermediateOutputPath)%(ManagedBinary.Filename).il" />
<IlcArg Condition="$(NoWarn) != ''" Include='--nowarn:"$([MSBuild]::Escape($(NoWarn)))"' />
<IlcArg Condition="$(TrimmerSingleWarn) == 'true'" Include="--singlewarn" />
<IlcArg Condition="$(IlcTrimMetadata) == 'true'" Include="--reflectedonly" />
<IlcArg Condition="$(IlcTrimMetadata) == 'false'" Include="--reflectableartifacts" />
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
<IlcArg Condition="'$(ControlFlowGuard)' == 'Guard' and '$(TargetOS)' == 'windows'" Include="--guard:cf" />
<IlcArg Include="@(_IlcRootedAssemblies->'--root:%(Identity)')" />
<IlcArg Include="@(_IlcConditionallyRootedAssemblies->'--conditionalroot:%(Identity)')" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ protected override MetadataCategory GetMetadataCategory(TypeDesc type)
return category;
}

protected override bool AllMethodsCanBeReflectable => (_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0;
protected override bool AllMethodsCanBeReflectable => (_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0;

protected override void ComputeMetadata(NodeFactory factory,
out byte[] metadataBlob,
Expand Down Expand Up @@ -494,7 +494,7 @@ protected override void GetDependenciesDueToMethodCodePresenceInternal(ref Depen
}

// Presence of code might trigger the reflectability dependencies.
if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0)
if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0)
{
GetDependenciesDueToReflectability(ref dependencies, factory, method);
}
Expand All @@ -505,7 +505,7 @@ public override void GetConditionalDependenciesDueToMethodCodePresence(ref Combi
MethodDesc typicalMethod = method.GetTypicalMethodDefinition();

// Ensure methods with genericness have the same reflectability by injecting a conditional dependency.
if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) != 0
if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) == 0
&& method != typicalMethod)
{
dependencies ??= new CombinedDependencyList();
Expand All @@ -516,7 +516,7 @@ public override void GetConditionalDependenciesDueToMethodCodePresence(ref Combi

public override void GetDependenciesDueToVirtualMethodReflectability(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0)
if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0)
{
// If we have a use of an abstract method, GetDependenciesDueToReflectability is not going to see the method
// as being used since there's no body. We inject a dependency on a new node that serves as a logical method body
Expand Down Expand Up @@ -1028,9 +1028,9 @@ public enum UsageBasedMetadataGenerationOptions
ReflectionILScanning = 4,

/// <summary>
/// Only members that were seen as reflected on will be reflectable.
/// Consider all native artifacts (native method bodies, etc) visible from reflection.
/// </summary>
ReflectedMembersOnly = 8,
CreateReflectableArtifacts = 8,

/// <summary>
/// Fully root used assemblies that are not marked IsTrimmable in metadata.
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/tools/aot/ILCompiler/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ internal class Program
private bool _noMetadataBlocking;
private bool _disableReflection;
private bool _completeTypesMetadata;
private bool _reflectedOnly;
private bool _reflectableartifacts;
private bool _scanReflection;
private bool _methodBodyFolding;
private int _parallelism = Environment.ProcessorCount;
Expand Down Expand Up @@ -203,7 +203,7 @@ private ArgumentSyntax ParseCommandLine(string[] args)
syntax.DefineOption("nometadatablocking", ref _noMetadataBlocking, "Ignore metadata blocking for internal implementation details");
syntax.DefineOption("disablereflection", ref _disableReflection, "Disable generation of reflection metadata");
syntax.DefineOption("completetypemetadata", ref _completeTypesMetadata, "Generate complete metadata for types");
syntax.DefineOption("reflectedonly", ref _reflectedOnly, "Generate metadata only for reflected members");
syntax.DefineOption("reflectableartifacts", ref _reflectableartifacts, "Consider generated native arifacts visible from reflection");
syntax.DefineOption("scanreflection", ref _scanReflection, "Scan IL for reflection patterns");
syntax.DefineOption("scan", ref _useScanner, "Use IL scanner to generate optimized code (implied by -O)");
syntax.DefineOption("noscan", ref _noScanner, "Do not use IL scanner to generate optimized code");
Expand Down Expand Up @@ -758,8 +758,8 @@ static string ILLinkify(string rootedAssembly)
metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CompleteTypesOnly;
if (_scanReflection)
metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectionILScanning;
if (_reflectedOnly)
metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectedMembersOnly;
if (_reflectableartifacts)
metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts;
if (_rootDefaultAssemblies)
metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.RootDefaultAssemblies;
}
Expand Down
8 changes: 6 additions & 2 deletions src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,10 @@ public static void Run()
Assert.Equal(1, typeof(TypeWithSpecificMethodKept).CountMethods());
Assert.Equal(1, typeof(TypeWithSpecificOverloadKept).CountMethods());
Assert.Equal(2, typeof(TypeWithAllOverloadsKept).CountMethods());
Assert.Equal(2, typeof(TestDynamicDependency).CountMethods());

// We only expect DependentMethod. We specifically don't expect to see the Run method (current method).
Assert.Equal(1, typeof(TestDynamicDependency).CountMethods());

Assert.Equal(1, typeof(TypeWithPublicPropertiesKept).CountProperties());
}
}
Expand Down Expand Up @@ -577,7 +580,8 @@ public static void Run()
{
// Check we detect these as intrinsics
IntPtr pBytes = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(ClassWithLayout1)));
object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2));
// https://github.com/dotnet/runtime/issues/70072
//object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2));
Marshal.DestroyStructure(pBytes, typeof(ClassWithLayout3));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs to be commented out too. You do not want to be calling DestroyStructure on uninitialized memory

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClassWithLayout3 is blittable, so the destroy structure stub is a no-op.

(Notice we also call PtrToStructure with ClassWithLayout2, not 3. It's misuse either way and the test relies on the fact that these stubs don't do much. We need to use different type for each call, otherwise we wouldn't notice if one of the intrinsics regresses. I don't expect this to be commented out for long. I just didn't want to fix it because I knew Vitek has a huge change in #70204 that the fix would conflict with and I didn't want to annoy him.)

Marshal.OffsetOf(typeof(ClassWithLayout4), "Field");

Expand Down
21 changes: 21 additions & 0 deletions src/tests/nativeaot/SmokeTests/DynamicGenerics/rd.xml
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,27 @@
</Method>
</Type>

<Type Name="CtorDict.MyType1">
<Method Name=".ctor" Dynamic="Required All" />
</Type>
<Type Name="CtorDict.MyType2">
<Method Name=".ctor" Dynamic="Required All" />
</Type>
<Type Name="CtorDict.MyType5">
<Method Name=".ctor" Dynamic="Required All" />
</Type>
<Type Name="CtorDict.MyType6">
<Method Name=".ctor" Dynamic="Required All" />
</Type>


<Type Name="ConstraintsTests+TypeWithPublicCtor">
<Method Name=".ctor" Dynamic="Required All" />
</Type>
<Type Name="MethodConstraintsTests+TypeWithPublicCtor">
<Method Name=".ctor" Dynamic="Required All" />
</Type>

<Type Name="MethodAndUnboxingStubTesting.GenericClass`2[[System.Object, netstandard],[System.Object, netstandard]]" Dynamic="Required All"/>
<Type Name="MethodAndUnboxingStubTesting.GenericClass2`2[[System.Object, netstandard],[System.Object, netstandard]]" Dynamic="Required All"/>
<Type Name="MethodAndUnboxingStubTesting.GenericStruct`2[[System.Object, netstandard],[System.Object, netstandard]]" Dynamic="Required All"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
<CLRTestPriority>0</CLRTestPriority>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CLRTestTargetUnsupported Condition="'$(IlcMultiModule)' == 'true'">true</CLRTestTargetUnsupported>

<!--
We rely on reflection metadata for .cctor to detect if a type was preinitialized
and this only works if presence of code implies presence of reflection metadata.
-->
<IlcTrimMetadata>false</IlcTrimMetadata>
</PropertyGroup>

<ItemGroup>
Expand Down
95 changes: 95 additions & 0 deletions src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ private static int Main()
TestNotReflectedIsNotReflectable.Run();
TestGenericInstantiationsAreEquallyReflectable.Run();
#endif
TestAttributeInheritance2.Run();
TestInvokeMethodMetadata.Run();
TestVTableOfNullableUnderlyingTypes.Run();
TestInterfaceLists.Run();

//
// Mostly functionality tests
Expand Down Expand Up @@ -1676,6 +1680,97 @@ public static void Run()
}
}

class TestAttributeInheritance2
{
[AttributeUsage(AttributeTargets.All, Inherited = true)]
class AttAttribute : Attribute { }

class Base
{
[Att]
public virtual void VirtualMethodWithAttribute() { }
}

class Derived : Base
{
public override void VirtualMethodWithAttribute() { }
}

public static void Run()
{
object[] attrs = typeof(Derived).GetMethod(nameof(Derived.VirtualMethodWithAttribute)).GetCustomAttributes(inherit: true);
if (attrs.Length != 1 || attrs[0].GetType().Name != nameof(AttAttribute))
{
throw new Exception();
}
}
}

class TestInvokeMethodMetadata
{
delegate int WithDefaultParameter1(int value = 1234);
delegate DateTime WithDefaultParameter2(DateTime value);

public static int Method(int value) => value;

public static DateTime Method(DateTime value) => value;

public static void Run()
{
// Check that we have metadata for the Invoke method to convert Type.Missing to the actual value.
WithDefaultParameter1 del1 = Method;
int val = (int)del1.DynamicInvoke(new object[] { Type.Missing });
if (val != 1234)
throw new Exception();

// Check that we have metadata for the Invoke method to find a matching method
Delegate del2 = Delegate.CreateDelegate(typeof(WithDefaultParameter2), typeof(TestInvokeMethodMetadata), nameof(Method));
if (del2.Method.ReturnType != typeof(DateTime))
throw new Exception();
}
}

class TestVTableOfNullableUnderlyingTypes
{
struct NeverAllocated
{
public override string ToString() => "Never allocated";
}

static Type s_hidden = typeof(Nullable<NeverAllocated>);

public static void Run()
{
// Underlying type of a Nullable needs to be considered constructed.
// Trimming warning suppressions in the libraries depend on this invariant.
var instance = RuntimeHelpers.GetUninitializedObject(s_hidden);
if (instance.ToString() != "Never allocated")
throw new Exception();
}
}

class TestInterfaceLists
{
interface IGeneric<T> { }

class Class<T> : IGeneric<T> { }

static Type s_hidden = typeof(Class<string>);

public static void Run()
{
// Can't drop an interface from the interface list if the interface is referenced.
// Trimming warning suppressions in the libraries depend on this invariant.
foreach (var intface in s_hidden.GetInterfaces())
{
if (intface.HasSameMetadataDefinitionAs(typeof(IGeneric<object>)))
return;
}

throw new Exception();
}
}

#region Helpers

private static Type GetTestType(string testName, string typeName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
<CLRTestKind>BuildAndRun</CLRTestKind>
<CLRTestPriority>0</CLRTestPriority>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants>$(DefineConstants);REFLECTION_FROM_USAGE</DefineConstants>

<!-- There's just too many of these warnings -->
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,18 @@
<CLRTestKind>BuildAndRun</CLRTestKind>
<CLRTestPriority>0</CLRTestPriority>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants>$(DefineConstants);REFLECTION_FROM_USAGE</DefineConstants>

<!-- There's just too many of these warnings -->
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<NoWarn>$(NoWarn);IL3050</NoWarn>

<!-- Look for MULTIMODULE_BUILD #define for the more specific incompatible parts -->
<CLRTestTargetUnsupported Condition="'$(IlcMultiModule)' == 'true'">true</CLRTestTargetUnsupported>

<IlcTrimMetadata>false</IlcTrimMetadata>
</PropertyGroup>
<ItemGroup>
<Compile Include="Reflection.cs" />
</ItemGroup>

<ItemGroup>
<IlcArg Include="--reflectedonly" />
</ItemGroup>
</Project>