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

ILLink produces invalid IL metadata with -b switch and Save assembly mode #86462

Closed
filipnavara opened this issue May 18, 2023 · 8 comments · Fixed by #87575
Closed

ILLink produces invalid IL metadata with -b switch and Save assembly mode #86462

filipnavara opened this issue May 18, 2023 · 8 comments · Fixed by #87575
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers

Comments

@filipnavara
Copy link
Member

filipnavara commented May 18, 2023

As part of testing dotnet/macios#18268 we hit an odd case where invalid IL is produced as the output of ILLink, resulting in later crash when post-processing the data with ILStrip. There's actually several issues at play here:

Repro:

  • Download extra-typeref-linker.zip
  • Update the illinktask.dll path in linker.rsp to an absolute path to the included task .dll; the custom task only changes the assembly action to Save since it cannot be done on command line
  • Run dotnet <path to illink.dll> @linker.rsp
  • Check out\extra-typeref.dll with the mdv tool (https://github.com/dotnet/metadata-tools) and observe that there's NFloat type reference that points to non-existent assembly reference

Observations:

  • Tested with .NET 7.0.302 and .NET 8.0 Preview 5 linkers. The issue exists in both. On .NET 8 the original test case produces different linker input and doesn't hit the problem, but the underlying bug still exists.
  • Removing the -b switch from linker.rsp makes the problem go away

cc @rolfbjarne @ivanpovazan

@filipnavara filipnavara added the area-Tools-ILLink .NET linker development as well as trimming analyzers label May 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 18, 2023
@ghost
Copy link

ghost commented May 18, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

As part of testing dotnet/macios#18268 we hit an odd case where invalid IL is produced as the output of ILLink, resulting in later crash when post-processing the data with ILStrip. There's actually several issues at play here:

Repro:

  • Download extra-typeref-linker.zip
  • Update the illinktask.dll path in linker.rsp to an absolute path to the included task .dll; the custom task only changes the assembly action to Save since it cannot be done on command line
  • Run dotnet <path to illink.dll> @linker.rsp
  • Check out\extra-typeref.dll with the mdv tool (https://github.com/dotnet/metadata-tools) and observe that there's NFloat type reference that points to non-existent assembly reference

Observations:

  • Tested with .NET 7.0.302 and .NET 8.0 Preview 5 linkers. The issue exists in both. On .NET 8 the original test case produces different linker input and doesn't hit the problem, but the underlying bug still exists.
  • Removing the -b switch from linker.rsp makes the problem go away

cc @rolfbjarne @ivanpovazan

Author: filipnavara
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

@filipnavara

This comment was marked as off-topic.

@filipnavara
Copy link
Member Author

I realized that I forgot to include the .pdb in the repro package. That's essential for the error to occur. Here's a fixed one:
extra-typeref-linker.zip

@filipnavara
Copy link
Member Author

filipnavara commented May 19, 2023

This fixes the output for me: filipnavara@7cca2fa

Unit tests will be easier to write once #86474 lands.

@filipnavara
Copy link
Member Author

This is similar to this old issue btw: dotnet/linker#399

@filipnavara
Copy link
Member Author

@vitek-karas This should be rather trivial to fix. I suppose that I can take a stab at it if you are busy...

@vitek-karas
Copy link
Member

I'm definitely busy with other commitments... maybe @sbomer can take a look.
Or if you want to tackle it - we would be happy to help of course.

@sbomer
Copy link
Member

sbomer commented Jun 9, 2023

Would love it if you were able to contribute a fix. :) Otherwise I can take a look next week.

@sbomer sbomer self-assigned this Jun 12, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2023
sbomer added a commit that referenced this issue Jun 16, 2023
#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.
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jun 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants