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

Linker produces colliding MVID for two different inputs #2203

Open
vitek-karas opened this issue Aug 11, 2021 · 11 comments
Open

Linker produces colliding MVID for two different inputs #2203

vitek-karas opened this issue Aug 11, 2021 · 11 comments

Comments

@vitek-karas
Copy link
Member

See dotnet/runtime#45649 for more details.

This is inherently a Cecil issue, but tracking it here since we don't normally watch Cecil repo.

Adding to 6.0.0 for now - to consider this at least.

/cc @agocke

@vitek-karas
Copy link
Member Author

#2384 brings the fix into main.
We should consider this for 6.0 servicing.

vitek-karas added a commit that referenced this issue Nov 18, 2021
The new Cecil fixes the deterministic MVID and PDB checksum.
Details in dotnet/cecil#31.
@filipnavara
Copy link
Member

We should consider this for 6.0 servicing.

Just a heads-up. We discussed an unrelated issue on Discord and found out that assemblies in different Mono runtime packs now get different MVID despite identical IL code.

For reference, see these packages:
https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.osx-x64/6.0.1 https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.osx-arm64/6.0.1

Check it for System.Collections.dll, it is likely identical. Now try the same on 7.0 nightly builds and it's no longer identical.

@vitek-karas
Copy link
Member Author

The new linker behavior should be:

  • Identical input produces output with deterministic (meaning always the same) MVID
  • Different input produces output with different MVID (this was not true before the fix as per above)

If anybody sees a different behavior then we need to look into that. So the question here is if the input to the linker was identical or not (note that identical IL is not enough, the above issue is that the dlls only differ in the architecture bit in the PE header, but otherwise have identical IL, those should still get different MVIDs).

The new linker also fixes the PDV checksum parts - so if for some reason the difference is only in the symbols, the dlls should not be identical (and should not get the same MVIDs).

@filipnavara
Copy link
Member

I agree it should be checked carefully. Some of the assemblies are originally built as AnyCPU so I would expect them to be identical if everything is deterministic (including PDB paths). Note that Xamarin.Android apparently does some deduplication by MVID so the determinism across architectures should be tested somehow.

vitek-karas added a commit to vitek-karas/linker that referenced this issue Mar 25, 2022
… in the fix for dotnet#2203 into 6.0.

The same fix has been in the linker/main for 2 months now and it shipped in 7.0 preview without any issues.
jonathanpeppers added a commit to dotnet/android that referenced this issue Mar 31, 2022
Context: dotnet/runtime#45649
Context: dotnet/linker#2203

We are getting unique MVIDs per architecture output from the linker.

To workaround this problem, I used the "size" of the files instead. This works for now, but is not 100% correct. I think it would be fine for .NET 7 Android apps to use this for now in main. (hopefully temporary)
@jonathanpeppers
Copy link
Member

I had to make a workaround for the Android workload in our .NET 7 PR:

dotnet/android@59a2630

Previously, many of the .NET assemblies output from the linker per architecture would have identical MVIDs. So for example System.Console.dll for android-arm64, android-x64, etc. would have identical MVIDs. The main assembly that differed in .NET 6 was System.Private.CoreLib.dll.

We use this info to put architecture specific assemblies in a subfolder in the final Android app, and so you end up with only a couple duplicated assemblies. What's happening now is every assembly is different, so I changed our logic (for now) to use file size instead of MVID. That seems to workaround this for now, but should we do something else instead?

@marek-safar
Copy link
Contributor

What's happening now is every assembly is different, so I changed our logic (for now) to use file size instead of MVID.

That sounds like a very bad idea. The same file size can hide so many differences especially with alignment.

@jonathanpeppers
Copy link
Member

Yes, I definitely want to revert this. I hope before merging this PR?

@agocke
Copy link
Member

agocke commented Mar 31, 2022

@filipnavara Can we get those two binaries to check? We need to validate that the linker didn't regress this before we can backport to 6.0

jonpryor pushed a commit to dotnet/android that referenced this issue May 5, 2022
Context: dotnet/runtime#56989
Context: dotnet/runtime#68734
Context: dotnet/runtime#68914
Context: dotnet/runtime#68701

Changes: dotnet/installer@04e40fa...c7afae6

	% git diff --shortstat 04e40fa9...c7afae69
	 98 files changed, 1788 insertions(+), 1191 deletions(-)

Changes: dotnet/runtime@a21b9a2...c5d40c9

	% git diff --shortstat a21b9a2d...c5d40c9e
	 28347 files changed, 1609359 insertions(+), 1066473 deletions(-)

Changes: dotnet/linker@01c4f59...04c49c9

	% git diff --shortstat 01c4f590...04c49c9d
	 577 files changed, 28039 insertions(+), 10835 deletions(-)

Updates to build with the .NET 7 SDK and use the runtime specified by
the SDK.  We no longer use different SDK & runtime versions.

This produces a 7.0.100 Android workload.

After this is merged we should be able to enable Maestro to consume
future .NET 7 builds from dotnet/installer/main.

~~ Known Issues ~~

AOT+LLVM crashes on startup:

  * dotnet/runtime#68914

Xamarin.Build.Download hits a breaking change with `ZipEntry`:

  * dotnet/runtime#68734
  * xamarin/XamarinComponents#1368

illink outputs different MVIDs per architecture:

  * dotnet/linker#2203
  * dotnet/runtime#67660

Size of `libmonosgen-2.0.so` regressed:

  * dotnet/runtime#68330
  * dotnet/runtime#68354

Newer .NET 7 builds crash on startup:

  * dotnet/runtime#68701
  * This is worked around by *disabling* lazy loading of AOT'd
    assemblies 6dc426f.
  * TODO: re-enable once we get a fixed .NET 7 runtime.

TODO: We can't yet push to the `dotnet7` feed. Working on this.

Co-authored-by: Jonathan Peppers <[email protected]>
Co-authored-by: Peter Collins <[email protected]>
@agocke
Copy link
Member

agocke commented Jul 6, 2022

@jonathanpeppers What's the status here? Are we clear to backport this to 6.0?

@jonathanpeppers
Copy link
Member

dotnet/runtime#67660 is still open. I think the plan was to make a reference assembly for System.Private.CoreLib.dll that the rest of the BCL could compile against. This would result in the BCL in the Android runtime packs having stable MVIDs across all architectures.

I don't think we can backport until this is done. We should probably do it for .NET 7/main, and if successful maybe that could be backported to .NET 6?

@agocke
Copy link
Member

agocke commented Jul 6, 2022

The change is already in for .NET 7, so it sounds like completing dotnet/runtime#67660 would be a ship blocker for .NET 7. I agree that we should do the work in main first, then backport.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Sep 9, 2022
Fixes: dotnet#7302
Context: dotnet/linker#2203
Context: dotnet/runtime#67660
Context: dotnet#6598

This partially backports 745214d.

In addition to this backport to dotnet/runtime/release/6.0:

dotnet/runtime#75311

We also have another change in .NET 7 that opts into
`$(TrimmerRemoveSymbols)` by default for `Release` builds. This allows
the .NET 7 linker to have stable MVIDs for assemblies for each
architecture.

There may potentially be a dotnet/linker issue here to look into
further. However, this seems to be the best fix for getting .NET 6
projects building under .NET 7 at the moment.
jonathanpeppers added a commit to dotnet/android that referenced this issue Sep 9, 2022
Fixes: #7302
Context: dotnet/linker#2203
Context: dotnet/runtime#67660
Context: #6598

This partially backports 745214d.

Building a `net6.0-android` app in `Release` mode with .NET 7 can fail with many errors like:

    error XA4215: The Java type `androidx.activity.contextaware.OnContextAvailableListener` is generated by more than one managed type. Please change the [Register] attribute so that the same Java type is not emitted.

This happens because we end up with multiple assemblies, such as:

    obj/Release/net6.0-android/android-arm/linked/Xamarin.AndroidX.Activity.dll
    obj/Release/net6.0-android/android-arm64/linked/Xamarin.AndroidX.Activity.dll
    obj/Release/net6.0-android/android-x64/linked/Xamarin.AndroidX.Activity.dll
    obj/Release/net6.0-android/android-x86/linked/Xamarin.AndroidX.Activity.dll

To fix this, in addition to this backport to dotnet/runtime/release/6.0:

dotnet/runtime#75311

In .NET 7 we opt into `$(TrimmerRemoveSymbols)` by default for `Release`
builds. This allows the .NET 7 linker to have stable MVIDs for assemblies
for each architecture. Somehow when the linker outputs `.pdb` files,
it creates different MVIDs per architecture.

There may potentially be a dotnet/linker issue here to look into
further. However, this seems to be the best fix for getting .NET 6
projects building under .NET 7 at the moment.
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
…otnet/linker#2384)

The new Cecil fixes the deterministic MVID and PDB checksum.
Details in dotnet/cecil#31.

Commit migrated from dotnet/linker@4f4a670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants