Fix missing assemblyref for typeref only kept for debug info in illink (

#87575)

Fixes #86462 by walking
debug info to discover typerefs in the assembly that are only
preserved due to debug info. In this case the assembly has the
'save' action, so the reference to the typeref in the debug info
is not swept. This means that the presence of the typeref in the
final output depends on whether we are linking the PDB. Walking
the typeref will preserve the assemblyref that it refers to,
fixing the issue.

The fix also needs to walk up the parent import scopes,
discovered by reproducing the issue in our test infra.

The behavior needs to depend on whether we are linking debug
symbols, otherwise we will keep the assemblyref (but not the
typeref that uses it) when PDBs are present, and the output
should not depend on the presence of
PDBs. `AssemblyOnlyUsedByUsingSaveAction` tests this case - see
comments in there for more detail.

This includes some unrelated test infra changes (supporting
multiple additional compiler arguments) which were useful for
iterating on this, even though they aren't necessary in the final
version of the testcases.

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

@@ -1452,7 +1452,7 @@ sealed class TypeReferenceMarker : TypeReferenceWalker
 			readonly MarkingHelpers markingHelpers;
 
 			TypeReferenceMarker (AssemblyDefinition assembly, MarkingHelpers markingHelpers)
-				: base (assembly)
+				: base (assembly, walkSymbols: false)
 			{
 				this.markingHelpers = markingHelpers;
 			}

src/tools/illink/src/linker/Linker.Steps/SweepStep.cs

@@ -58,7 +58,7 @@ protected override void Process ()
 			foreach (var assembly in assemblies)
 				RemoveUnmarkedAssembly (assembly);
 
-			// Look for references (included to previously unresolved assemblies) marked for deletion
+			// Look for references (including to previously unresolved assemblies) marked for deletion
 			foreach (var assembly in assemblies)
 				UpdateAssemblyReferencesToRemovedAssemblies (assembly);
 
@@ -69,7 +69,7 @@ protected override void Process ()
 				case AssemblyAction.CopyUsed:
 				case AssemblyAction.Link:
 				case AssemblyAction.Save:
-					bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					bool changed = SweepAssemblyReferences (assembly);
 					if (changed && action == AssemblyAction.CopyUsed)
 						Annotations.SetAction (assembly, AssemblyAction.Save);
 					break;
@@ -174,7 +174,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly)
 				AssemblyAction assemblyAction = AssemblyAction.Copy;
 				if (SweepTypeForwarders (assembly)) {
 					// Need to sweep references, in case sweeping type forwarders removed any
-					AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					SweepAssemblyReferences (assembly);
 					assemblyAction = AssemblyAction.Save;
 				}
 
@@ -194,7 +194,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly)
 			case AssemblyAction.Save:
 				if (SweepTypeForwarders (assembly)) {
 					// Need to sweep references, in case sweeping type forwarders removed any
-					AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					SweepAssemblyReferences (assembly);
 				}
 				break;
 			}
@@ -242,7 +242,7 @@ protected virtual void SweepAssembly (AssemblyDefinition assembly)
 			}
 
 			if (SweepTypeForwarders (assembly) || updateScopes)
-				AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+				SweepAssemblyReferences (assembly);
 		}
 
 		bool IsMarkedAssembly (AssemblyDefinition assembly)
@@ -568,32 +568,32 @@ protected virtual void CustomAttributeUsageRemoved (ICustomAttributeProvider pro
 		{
 		}
 
+		bool SweepAssemblyReferences (AssemblyDefinition assembly)
+		{
+			//
+			// We used to run over list returned by GetTypeReferences but
+			// that returns typeref(s) of original assembly and we don't track
+			// which types are needed for which assembly which left us
+			// with dangling assembly references
+			//
+			assembly.MainModule.AssemblyReferences.Clear ();
+
+			var arc = new AssemblyReferencesCorrector (assembly, walkSymbols: Context.LinkSymbols);
+			arc.Process ();
+
+			return arc.ChangedAnyScopes;
+		}
+
 		sealed class AssemblyReferencesCorrector : TypeReferenceWalker
 		{
 			readonly DefaultMetadataImporter importer;
 
-			bool changedAnyScopes;
+			public bool ChangedAnyScopes { get; private set; }
 
-			AssemblyReferencesCorrector (AssemblyDefinition assembly) : base (assembly)
+			public AssemblyReferencesCorrector (AssemblyDefinition assembly, bool walkSymbols) : base (assembly, walkSymbols)
 			{
 				this.importer = new DefaultMetadataImporter (assembly.MainModule);
-				changedAnyScopes = false;
-			}
-
-			public static bool SweepAssemblyReferences (AssemblyDefinition assembly)
-			{
-				//
-				// We used to run over list returned by GetTypeReferences but
-				// that returns typeref(s) of original assembly and we don't track
-				// which types are needed for which assembly which left us
-				// with dangling assembly references
-				//
-				assembly.MainModule.AssemblyReferences.Clear ();
-
-				var arc = new AssemblyReferencesCorrector (assembly);
-				arc.Process ();
-
-				return arc.changedAnyScopes;
+				ChangedAnyScopes = false;
 			}
 
 			protected override void ProcessTypeReference (TypeReference type)
@@ -626,7 +626,7 @@ protected override void ProcessTypeReference (TypeReference type)
 					return;
 
 				type.Scope = tr.Scope;
-				changedAnyScopes = true;
+				ChangedAnyScopes = true;
 			}
 
 			protected override void ProcessExportedType (ExportedType exportedType)
@@ -647,7 +647,7 @@ protected override void ProcessExportedType (ExportedType exportedType)
 					return;
 
 				exportedType.Scope = tr.Scope;
-				changedAnyScopes = true;
+				ChangedAnyScopes = true;
 			}
 		}
 	}

src/tools/illink/src/linker/Linker/TypeReferenceWalker.cs

@@ -15,15 +15,18 @@ abstract class TypeReferenceWalker
 
 		protected HashSet<TypeReference> Visited { get; } = new HashSet<TypeReference> ();
 
-		public TypeReferenceWalker (AssemblyDefinition assembly)
+		readonly bool walkSymbols;
+
+		public TypeReferenceWalker (AssemblyDefinition assembly, bool walkSymbols)
 		{
 			this.assembly = assembly;
+			this.walkSymbols = walkSymbols;
 		}
 
 		// Traverse the assembly and mark the scopes of discovered type references (but not exported types).
 		// This includes scopes referenced by Cecil TypeReference objects that don't represent rows in the typeref table,
 		// such as references to built-in types, or attribute arguments which encode type references as strings.
-		protected virtual void Process ()
+		public virtual void Process ()
 		{
 			if (Visited.Count > 0)
 				throw new InvalidOperationException ();
@@ -103,6 +106,9 @@ void WalkScopes (TypeDefinition typeDefinition)
 
 					if (m.HasBody)
 						WalkTypeScope (m.Body);
+
+					if (walkSymbols && m.DebugInformation?.Scope?.Import is ImportDebugInformation import)
+						WalkDebugInfoImportScope (import);
 				}
 			}
 
@@ -315,6 +321,17 @@ void WalkForwardedTypesScope (CustomAttributeArgument attributeArgument)
 			}
 		}
 
+		void WalkDebugInfoImportScope (ImportDebugInformation import)
+		{
+			if (import.HasTargets) {
+				foreach (var target in import.Targets)
+					WalkScopeOfTypeReference (target.Type);
+			}
+
+			if (import.Parent is ImportDebugInformation parent)
+				WalkDebugInfoImportScope (parent);
+		}
+
 		void WalkScopeOfTypeReference (TypeReference type)
 		{
 			if (type == null)

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs

@@ -11,7 +11,7 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 	[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
 	public class SetupCompileBeforeAttribute : BaseMetadataAttribute
 	{
-		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false, string outputSubFolder = null)
+		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string[] additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false, string outputSubFolder = null)
 		{
 			ArgumentNullException.ThrowIfNull (sourceFiles);
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj

@@ -16,6 +16,7 @@
     <Compile Remove="LinkXml\Dependencies\CanPreserveAnExportedType_Forwarder.cs" />
     <Compile Remove="LinkXml\Dependencies\UsedNonRequiredExportedTypeIsKept_fwd.cs" />
     <Compile Remove="LinkXml\Dependencies\UsedNonRequiredExportedTypeIsKeptWhenRooted_fwd.cs" />
+    <Compile Remove="References\Dependencies\CustomMarkHandlerSaveAssembly.cs" />
     <Compile Remove="TypeForwarding\Dependencies\AnotherLibraryForwarder.cs" />
     <Compile Remove="TypeForwarding\Dependencies\AssemblyReferenceIsRemovedWhenUnused_Library.cs" />
     <Compile Remove="TypeForwarding\Dependencies\ForwarderLibrary.cs" />

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingSaveAction.cs

@@ -0,0 +1,42 @@
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Helpers;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+using Mono.Linker.Tests.Cases.References.Dependencies;
+
+namespace Mono.Linker.Tests.Cases.References
+{
+	/// <summary>
+	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
+	/// With the assembly action of `save`, we remove unused assembly references from the assembly and rewrite it.
+	/// When cecil writes the assembly, the unused typeref is not written out.
+	/// </summary>
+
+	// Add a custom step which sets the assembly action of the test to "save"    
+    [SetupCompileBefore ("SetSaveAction.dll", new[] { "Dependencies/CustomMarkHandlerSaveAssembly.cs" },
+		new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })]
+    [SetupLinkerArgument ("--custom-step", "CustomMarkHandlerSaveAssembly,SetSaveAction.dll")]
+
+	// When csc is used, `saved.dll` will have a reference to `library.dll`
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("saved.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" },
+		// Even though this testcase doesn't link symbols, tell the compiler to produce symbols to confirm that the behavior
+		// isn't affected by the presence of symbols when not passing '-b'.
+		new[] { "library.dll" }, additionalArguments: new string[] { "/debug:portable" }, compilerToUse: "csc")]
+
+	// Here to assert that the test is setup correctly to preserve unused code in the saved assembly.  This is an important aspect of the bug
+	[KeptMemberInAssembly ("saved.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
+
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	[RemovedAssembly ("library.dll")]
+	// The `save` action results in the reference to System.Runtime being resolved into a reference directly to System.Private.CoreLib.
+	// The reference to `library` is removed.
+	[KeptReferencesInAssembly ("saved.dll", new[] { "System.Private.CoreLib" })]
+	public class AssemblyOnlyUsedByUsingSaveAction
+	{
+		public static void Main ()
+		{
+			// Use something to keep the reference at compile time
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
+		}
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingSaveActionWithSymbols.cs

@@ -0,0 +1,45 @@
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Helpers;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+using Mono.Linker.Tests.Cases.References.Dependencies;
+
+namespace Mono.Linker.Tests.Cases.References
+{
+	/// <summary>
+	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
+	/// With the assembly action of `save`, we remove unused assembly references from the assembly and rewrite it, preserving all methods.
+	/// When debug symbols are present for the `save` assembly, we do not trim debug symbols, so cecil keeps all type references in the
+	/// save assembly that are referenced by the debug info. This includes the unused typeref, so the assemblyref referenced by the typeref
+	/// must also be preserved.
+	/// </summary>
+
+	// Add a custom step which sets the assembly action of the test to "save"    
+    [SetupCompileBefore ("SetSaveAction_Symbols.dll", new[] { "Dependencies/CustomMarkHandlerSaveAssembly.cs" },
+		new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })]
+    [SetupLinkerArgument ("--custom-step", "CustomMarkHandlerSaveAssembly,SetSaveAction_Symbols.dll")]
+
+	// When csc is used, `saved.dll` will have a reference to `library.dll`
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("saved.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" },
+		new[] { "library.dll" }, additionalArguments: new string[] { "/debug:portable" }, compilerToUse: "csc")]
+
+	// Here to assert that the test is setup correctly to preserve unused code in the saved assembly.  This is an important aspect of the bug
+	[KeptMemberInAssembly ("saved.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
+
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	[RemovedAssembly ("library.dll")]
+	// The `save` action results in the reference to System.Runtime being resolved into a reference directly to System.Private.CoreLib.
+	// The reference to `library` is kept, because it is referenced from a typeref that is referenced from the debug info.
+	[KeptReferencesInAssembly ("saved.dll", new[] { "System.Private.CoreLib", "library" })]
+
+	// Linking debug symbols is required for cecil not to remove the typeref from the assembly, because it is only referenced from the debug info.
+	[SetupLinkerLinkSymbols ("true")]
+	public class AssemblyOnlyUsedByUsingSaveActionWithSymbols
+	{
+		public static void Main ()
+		{
+			// Use something to keep the reference at compile time
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
+		}
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingWithCsc.cs

@@ -7,26 +7,26 @@ namespace Mono.Linker.Tests.Cases.References
 {
 	/// <summary>
 	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
-	/// Because of that, `copied` needs to have it's reference to `library` removed even though we specified an assembly action of `copy`
+	/// However, because we specified an assembly action of `copy`, we do not rewrite `copied`, and it ends up with an unused reference to the removed `library`.
 	/// </summary>
 	[SetupLinkerAction ("copy", "copied")]
-	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
 
 	// When csc is used, `copied.dll` will have a reference to `library.dll`
-	[SetupCompileBefore ("copied.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Copied.cs" }, new[] { "library.dll" }, compilerToUse: "csc")]
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("copied.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" }, new[] { "library.dll" }, compilerToUse: "csc")]
 
 	// Here to assert that the test is setup correctly to copy the copied assembly.  This is an important aspect of the bug
-	[KeptMemberInAssembly ("copied.dll", typeof (AssemblyOnlyUsedByUsing_Copied), "Unused()")]
+	[KeptMemberInAssembly ("copied.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
 
-	// We library should be gone.  The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
 	[RemovedAssembly ("library.dll")]
 	[KeptReferencesInAssembly ("copied.dll", new[] { "System.Runtime", "library" })]
 	public class AssemblyOnlyUsedByUsingWithCsc
 	{
 		public static void Main ()
 		{
 			// Use something to keep the reference at compile time
-			AssemblyOnlyUsedByUsing_Copied.UsedToKeepReference ();
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/AssemblyOnlyUsedByUsing_Copied.cs → src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs

@@ -1,11 +1,10 @@
-
 // This is what triggers the behavior difference between Roslyn and mcs.  Roslyn will keep the reference
 // to this assembly because of this whereas mcs will not
 using ImportantForBug = Mono.Linker.Tests.Cases.References.Dependencies.AssemblyOnlyUsedByUsing_Lib;
 
 namespace Mono.Linker.Tests.Cases.References.Dependencies
 {
-	public class AssemblyOnlyUsedByUsing_Copied
+	public class AssemblyOnlyUsedByUsing_UnusedUsing
 	{
 		public static void UsedToKeepReference ()
 		{
@@ -15,4 +14,4 @@ private static void Unused ()
 		{
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/CustomMarkHandlerSaveAssembly.cs

@@ -0,0 +1,15 @@
+using System;
+using Mono.Cecil;
+using Mono.Linker;
+using Mono.Linker.Steps;
+
+public class CustomMarkHandlerSaveAssembly : IMarkHandler
+{
+    public void Initialize (LinkContext context, MarkContext markContext)
+    {
+        markContext.RegisterMarkAssemblyAction (assembly => {
+            if (assembly.Name.Name == "saved")
+                context.Annotations.SetAction (assembly, AssemblyAction.Save);
+        });
+    }
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdb.cs

@@ -5,7 +5,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("false")]
 
 	[RemovedSymbols ("LibraryWithEmbeddedPdbSymbols.dll")]

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabled.cs

@@ -5,7 +5,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 
 	[KeptSymbols ("LibraryWithEmbeddedPdbSymbols.dll")]

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabledAndDeterministicMvid.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 	[SetupLinkerArgument ("--deterministic", "true")]
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabledAndNewMvid.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 	[SetupLinkerArgument ("--deterministic", "true")]
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbCopyAction.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("false")]
 	[SetupLinkerAction ("copy", "LibraryWithEmbeddedPdbSymbols")]