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

Avoid copy action for root assemblies #96363

Merged
merged 21 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ public static bool IsMetadataTokenSupported
// Changed to `true` when trimming
public static bool IsBuiltWithAggressiveTrimming => IsNativeAot;
public static bool IsNotBuiltWithAggressiveTrimming => !IsBuiltWithAggressiveTrimming;
public static bool IsTrimmedWithILLink => IsBuiltWithAggressiveTrimming && !IsNativeAot;

// Windows - Schannel supports alpn from win8.1/2012 R2 and higher.
// Linux - OpenSsl supports alpn from openssl 1.0.2 and higher.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,20 @@
<TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptors.xml" />
</ItemGroup>

<Target Name="PreserveEnCAssembliesFromLinking" Condition="'$(TargetOS)' == 'browser' and '$(EnableAggressiveTrimming)' == 'true'" BeforeTargets="ConfigureTrimming">
<Target Name="PreserveEnCAssembliesFromLinking" Condition="'$(TargetOS)' == 'browser' and '$(EnableAggressiveTrimming)' == 'true'" BeforeTargets="PrepareForILLink">
<ItemGroup>
<!-- want to compute the intersection: apply update test assemblies that are also resolved to publish.
<!-- want to compute the intersection: apply update test assemblies that are also resolved to link.
-->
<ApplyUpdateTestProjectReference Include="@(ProjectReference)" Condition="$([System.String]::new('%(FileName)').StartsWith('$(ApplyUpdateTestPrefix)'))" />
<ApplyUpdateTestAssemblyName Include="@(ApplyUpdateTestProjectReference->'%(FileName).dll')" />
<ResolvedFileToPublishNoDirName Include="%(ResolvedFileToPublish.FileName)%(ResolvedFileToPublish.Extension)">
<ResolvedFileToPublish>%(ResolvedFileToPublish.FullPath)</ResolvedFileToPublish>
</ResolvedFileToPublishNoDirName>
<ResolvedApplyUpdateAssembly Include="@(ResolvedFileToPublishNoDirName)" Condition="'@(ResolvedFileToPublishNoDirName)' == '@(ApplyUpdateTestAssemblyName)' and %(Identity) != ''" />
<!-- Don't let the IL linker modify EnC test assemblies -->
<TrimmerRootAssembly Include="%(ResolvedApplyUpdateAssembly.ResolvedFileToPublish)" />
</ItemGroup>
<ManagedAssemblyToLink Condition="'%(FileName)%(Extension)' == '@(ApplyUpdateTestAssemblyName)'">
<TrimMode>copy</TrimMode>
</ManagedAssemblyToLink>
<ManagedAssemblyToLink Condition="'%(FileName)%(Extension)' == '@(IntermediateAssembly->'%(FileName)%(Extension)')'">
<TrimMode>copy</TrimMode>
</ManagedAssemblyToLink>
</ItemGroup>
</Target>

<Target Name="IncludeDeltasInWasmBundle" BeforeTargets="PrepareForWasmBuildApp" Condition="'$(TargetOS)' == 'browser'">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ public void MethodInfoReflectedTypeDoesNotSurviveRuntimeHandles()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtimelab/issues/830", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming)),
/* Stripping ComVisible because descriptors tell us so */]
public void GetCustomAttributesData()
{
MemberInfo[] m = typeof(MemberInfoTests).GetMember("SampleClass");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

<!-- The test is looking for debugger attributes we would have stripped with NativeAOT -->
<IlcKeepManagedDebuggerSupport>true</IlcKeepManagedDebuggerSupport>
<!-- Same for ILLink -->
<DebuggerSupport>true</DebuggerSupport>
</PropertyGroup>

<!--
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class ChildAttributeWithField : ParentAttribute
public int Field = 0;
}

[ActiveIssue("https://github.com/dotnet/runtimelab/issues/803", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
[ActiveIssue("https://github.com/dotnet/runtimelab/issues/803", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming))]
[Fact]
[StringValue("\uDFFF")]
public static void StringArgument_InvalidCodeUnits_FallbackUsed()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public void TestIsDefined()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot) /* Stripping ComVisible because descriptors tell us so */)]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming)
/* Stripping ComVisible because descriptors tell us so */)]
public void IsDefined_PropertyInfo()
{
PropertyInfo pi = typeof(TestBase).GetProperty("PropBase3");
Expand Down Expand Up @@ -327,6 +328,8 @@ public static class GetCustomAttribute
{

[Fact]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsTrimmedWithILLink))
Copy link
Member

Choose a reason for hiding this comment

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

Wondered why this wasn't hit with naot and found out illinker has this as a hardcoded list. Weird, but okay I guess. Would be nice to move this to the same plan as all the other attributes at least. I'm not convinced anyone will want to work on 2078 anytime soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #96907

/* Stripping security attributes removes UnverifiableCodeAttribute */]
public static void customAttributeCount()
{
List<CustomAttributeData> customAttributes = typeof(GetCustomAttribute).Module.CustomAttributes.ToList();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ public static void Test_CustomAttribute_Constructor_CrossAssembly1()

[Fact]
[ComVisible(false)]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot)) /* Descriptors tell us to remove ComVisibleAttribute */]
[ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming))
/* Descriptors tell us to remove ComVisibleAttribute */]
public static void Test_CustomAttribute_Constructor_CrossAssembly2()
{
MethodInfo m = (MethodInfo)MethodBase.GetCurrentMethod();
Expand Down
12 changes: 12 additions & 0 deletions src/tools/illink/src/linker/CompatibilitySuppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,12 @@
<Left>ref/net8.0/illink.dll</Left>
<Right>lib/net8.0/illink.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.IsRootAssembly(Mono.Cecil.AssemblyDefinition)</Target>
<Left>ref/net8.0/illink.dll</Left>
<Right>lib/net8.0/illink.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.Mark(Mono.Cecil.CustomAttribute,Mono.Linker.DependencyInfo@)</Target>
Expand Down Expand Up @@ -1243,6 +1249,12 @@
<Left>ref/net8.0/illink.dll</Left>
<Right>lib/net8.0/illink.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.SetRootAssembly(Mono.Cecil.AssemblyDefinition)</Target>
<Left>ref/net8.0/illink.dll</Left>
<Right>lib/net8.0/illink.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.SetSubstitutedInit(Mono.Cecil.TypeDefinition)</Target>
Expand Down
20 changes: 13 additions & 7 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
DependencyKind.InterfaceImplementationInterfaceType,
DependencyKind.Ldtoken,
DependencyKind.ModifierType,
DependencyKind.NestedType,
DependencyKind.InstructionTypeRef,
DependencyKind.ParameterType,
DependencyKind.ReferencedBySpecialAttribute,
Expand Down Expand Up @@ -373,7 +374,7 @@ internal void MarkEntireType (TypeDefinition type, in DependencyInfo reason)
MarkEntireType (nested, new DependencyInfo (DependencyKind.NestedType, type));
}

Annotations.Mark (type, reason, ScopeStack.CurrentScope.Origin);
MarkTypeVisibleToReflection (type, type, reason, ScopeStack.CurrentScope.Origin);
MarkCustomAttributes (type, new DependencyInfo (DependencyKind.CustomAttribute, type));
MarkTypeSpecialCustomAttributes (type);

Expand All @@ -386,26 +387,27 @@ internal void MarkEntireType (TypeDefinition type, in DependencyInfo reason)

if (type.HasFields) {
foreach (FieldDefinition field in type.Fields) {
MarkField (field, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
MarkFieldVisibleToReflection (field, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
}
}

if (type.HasMethods) {
foreach (MethodDefinition method in type.Methods) {
Annotations.SetAction (method, MethodAction.ForceParse);
MarkMethod (method, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
if (IsFullyPreservedAction (Annotations.GetAction (type.Module.Assembly)))
Annotations.SetAction (method, MethodAction.ForceParse);
MarkMethodVisibleToReflection (method, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
}
}

if (type.HasProperties) {
foreach (var property in type.Properties) {
MarkProperty (property, new DependencyInfo (DependencyKind.MemberOfType, type));
MarkPropertyVisibleToReflection (property, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
}
}

if (type.HasEvents) {
foreach (var ev in type.Events) {
MarkEvent (ev, new DependencyInfo (DependencyKind.MemberOfType, type));
MarkEventVisibleToReflection (ev, new DependencyInfo (DependencyKind.MemberOfType, type), ScopeStack.CurrentScope.Origin);
}
}
}
Expand Down Expand Up @@ -1422,6 +1424,8 @@ protected virtual void MarkAssembly (AssemblyDefinition assembly, DependencyInfo
MarkEntireAssembly (assembly);
}
return;
} else if (Annotations.IsRootAssembly (assembly)) {
MarkEntireAssembly (assembly);
}

ProcessModuleType (assembly);
Expand Down Expand Up @@ -2033,7 +2037,9 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in
// will call MarkType on the attribute type itself).
// If for some reason we do keep the attribute type (could be because of previous reference which would cause IL2045
// or because of a copy assembly with a reference and so on) then we should not spam the warnings due to the type itself.
if (!(reason.Source is IMemberDefinition sourceMemberDefinition && sourceMemberDefinition.DeclaringType == type))
// Also don't warn when the type is marked due to an assembly being rooted.
if (!(reason.Source is IMemberDefinition sourceMemberDefinition && sourceMemberDefinition.DeclaringType == type) &&
reason.Kind is not DependencyKind.TypeInAssembly)
Context.LogWarning (ScopeStack.CurrentScope.Origin, DiagnosticId.AttributeIsReferencedButTrimmerRemoveAllInstances, type.GetDisplayName ());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ protected override void Process ()
Context.MetadataTrimming = MetadataTrimming.None;
break;
case AssemblyRootMode.AllMembers:
Context.Annotations.SetAction (assembly, AssemblyAction.Copy);
Annotations.SetRootAssembly (assembly);
Annotations.Mark (assembly.MainModule, di, origin);
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
Expand Down Expand Up @@ -1583,7 +1583,7 @@ public bool Analyze (in CalleePayload callee, Stack<MethodDefinition> callStack)
return false;

if (operand is int oint) {
if (oint == 1)
if (oint != 0)
jmpTarget = (Instruction) instr.Operand;
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed? It's a good change, but surprised to see this in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I should've separated it out, but I thought this change was small enough to include - allowing branch removal caused this test to fail, due to this bug. More detail in the commit message for aadf946.


continue;
Expand Down
11 changes: 11 additions & 0 deletions src/tools/illink/src/linker/Linker/Annotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public partial class AnnotationStore
protected readonly Dictionary<TypeDefinition, TypePreserveMembers> preserved_type_members = new ();
protected readonly Dictionary<ExportedType, TypePreserveMembers> preserved_exportedtype_members = new ();
protected readonly Dictionary<IMemberDefinition, List<MethodDefinition>> preserved_methods = new Dictionary<IMemberDefinition, List<MethodDefinition>> ();
readonly HashSet<AssemblyDefinition> assemblies_with_root_all_members = new ();
protected readonly HashSet<IMetadataTokenProvider> public_api = new HashSet<IMetadataTokenProvider> ();
protected readonly Dictionary<AssemblyDefinition, ISymbolReader> symbol_readers = new Dictionary<AssemblyDefinition, ISymbolReader> ();
readonly Dictionary<IMemberDefinition, LinkerAttributesInformation> linker_attributes = new Dictionary<IMemberDefinition, LinkerAttributesInformation> ();
Expand Down Expand Up @@ -399,6 +400,16 @@ public bool TryGetPreservedMembers (ExportedType type, out TypePreserveMembers p
return preserved_exportedtype_members.TryGetValue (type, out preserve);
}

public void SetRootAssembly (AssemblyDefinition assembly)
{
assemblies_with_root_all_members.Add (assembly);
}

public bool IsRootAssembly (AssemblyDefinition assembly)
{
return assemblies_with_root_all_members.Contains (assembly);
}

public bool TryGetMethodStubValue (MethodDefinition method, out object? value)
{
return MemberActions.TryGetMethodStubValue (method, out value);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;

namespace Mono.Linker.Tests.Cases.Expectations.Metadata
{
[AttributeUsage (AttributeTargets.Class, AllowMultiple = false)]
public class SetupLinkerLinkAllAttribute : BaseMetadataAttribute
{
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Mono.Linker.Tests.Cases.Expectations.Assertions;
using Mono.Linker.Tests.Cases.Expectations.Metadata;

Expand All @@ -6,12 +9,12 @@ namespace Mono.Linker.Tests.Cases.Libraries
[IgnoreTestCase ("NativeAOT doesn't implement library trimming the same way", IgnoredBy = Tool.NativeAot)]
[KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)]
[SetupCompileAsLibrary]
[SetupLinkerArgument ("-a", "test.dll")]
[SetupLinkerLinkAll]
[Kept]
[KeptMember (".ctor()")]
public class DefaultLibraryLinkBehavior
{
// Kept because by default libraries their action set to copy
// Kept because by default libraries have all members rooted
[Kept]
public static void Main ()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Mono.Linker.Tests.Cases.Libraries.Dependencies;

#if RootAllLibrary
[assembly: TypeForwardedTo (typeof (RootAllLibrary_ExportedType))]
#endif

namespace Mono.Linker.Tests.Cases.Libraries.Dependencies
{
public class RootAllLibrary
{
public static void Public ()
{
}

private static void Private ()
{
}

private class NestedType
{
}

public static void RemovedBranch ()
{
if (SubstitutedProperty)
RootAllLibrary_OptionalDependency.Use ();
}

// Substituted to false in RootAllLibrary_Substitutions.xml
static bool SubstitutedProperty {
get {
RequiresUnreferencedCode ();
return true;
}
}

[RequiresUnreferencedCode (nameof (RequiresUnreferencedCode))]
static void RequiresUnreferencedCode ()
{
}

[RootAllLibrary_RemovedAttribute]
class TypeWithRemovedAttribute
{
}
}

class NonPublicType
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Mono.Linker.Tests.Cases.Libraries.Dependencies
{
public class RootAllLibrary_ExportedType
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<linker>
<assembly fullname="removedattribute">
<type fullname="Mono.Linker.Tests.Cases.Libraries.Dependencies.RootAllLibrary_RemovedAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System.Diagnostics.CodeAnalysis;

namespace Mono.Linker.Tests.Cases.Libraries.Dependencies
{
public class RootAllLibrary_OptionalDependency
{
[RequiresUnreferencedCode (nameof (RootAllLibrary_OptionalDependency))]
public static void Use ()
{
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using System;

namespace Mono.Linker.Tests.Cases.Libraries.Dependencies
{
public class RootAllLibrary_RemovedAttribute : Attribute
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<linker>
<assembly fullname="library">
<type fullname="Mono.Linker.Tests.Cases.Libraries.Dependencies.RootAllLibrary">
<method signature="System.Boolean get_SubstitutedProperty()" body="stub" value="false" />
</type>
</assembly>
</linker>
Loading
Loading