Fix missing assemblyref for typeref only kept for debug info in illink #87575
Conversation
When linking `save` assemblis with `-b true` (linking debug info), we preserve all debug info (and don't sweep the debug info like we do for `link` assemblies). With this change, we now take into account type references that are only referenced by the debug info, preserving any assembly refs needed by them. This adds test infra to walk the type references in the output assembly, and check that they reference assembly refs that also exist in the output. Includes some test infra changes that were ultimately not necessary for the repro, but were useful, to allow passing multiple additional compiler args when compiling testcase dependencies.
This should be adequately covered by NoLinkedOutputAttribute
ghost
added
the
linkable-framework
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue DetailsFixes #86462 using the approach suggested in #86462 (comment). Initially I was looking into removing the unused typeref, but since we don't sweep debug info for save assemblies, I think it makes sense to keep it, even though this means the presence of the typeref in the final output depends on whether we are linking the PDB. The fix also needed 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 I don't think the output should depend on the presence of PDBs. 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.
|
|
Yeah, reproducing this in a unit test was challenging. Thanks a lot for the detailed issue report! |
src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
Outdated
Show resolved
Hide resolved
|
It is a bit weird, but it's at least consistent with what we do for the other kept typerefs in save assemblies. It's also a bit weird that if we aren't linking debug symbols, we will actually remove both the typeref and the assemblyref in this example - but I think it makes sense for the reason I gave above. |
|
I perfectly fine with the behavior regarding stripping symbols - I think it makes sense. |
Fixes #86462 using the approach suggested in #86462 (comment). Initially I was looking into removing the unused typeref, but since we don't sweep debug info for save assemblies, I think it makes sense to keep it, even though this means the presence of the typeref in the final output depends on whether we are linking the PDB.
The fix also needed 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 I don't think the output should depend on the presence of PDBs.
AssemblyOnlyUsedByUsingSaveActiontests 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.