-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Fix deterministic MVID and add PdbChecksum #810
Conversation
Nice! This should fix #610. |
Thanks a lot for the PR, I'll review ASAP. |
does the mono side have tests that can also be ported over? |
@SimonCropp this PR adds quite a few tests: https://github.com/jbevain/cecil/pull/810/files#diff-a834b6b7fab0440c40cd010733a632eec5086a9b39422a5a36f11f1f7dc5ed78 What other tests do you have in mind? (The way we hit this is actually long way away from any tests - users reported that in some cases symbols are not loading in VS and if we were to add tests for that it would be something like "the assemblies produced by official builds of dotnet/runtime all have valid symbols on the symbol server. We didn't hit this through automated testing - that is a test hole though). |
@vitek-karas so sorry. i only glanced at the latest commit in my GH notification emial. |
@vitek-karas Thank you for all this work. I spent a bunch of time reading through the changes and I think I'm mostly OK with it. One request though, I've started some initial work a while ago: https://github.com/jbevain/cecil/pull/617/files This introduces a Write method on the debug writer, which makes it a lot clearer than expecting GetDebugHeader to do the writing. I'd love it if you could incorporate the logic from the initial work into your PR as I think it makes things a bit clearer. Thanks! |
@jbevain Thanks for the review! I did see your original change and the If making such a breaking change is fine, I'll be happy to change the implementation to use the |
I think that's an acceptable breakage, I'm not aware of any other implementor :) |
This mostly cleans up the code to make it easier to understand. `ISymbolWriter.GetDebugHeader` no longer actually writes the symbols, there's a new `Write` method for just that. The assembly writer calls `Write` first and then the image writer calls `GetDebugHeader` when it's needed. This is partially taken from jbevain#617.
Made the changes to add Could you please rerun the Linux CI? The failure seems completely unrelated to my change. |
Thanks! |
Context: #9043 Changes: jbevain/cecil@0.11.4...0.11.5 * jbevain/cecil@8c123e1: Bump to 0.11.5 * jbevain/cecil@870ce3e: Fix RVA field alignment (jbevain/cecil#888) * jbevain/cecil@4ad9c0f: Fix that method resolution would consider all function-pointers to be the same (jbevain/cecil#885) * jbevain/cecil@cc48622: Fix a StackOverflowException reading windows runtime assemblies. (jbevain/cecil#879) * jbevain/cecil@341fb14: Treat instance and static methods as different methods during resolution (jbevain/cecil#882) * jbevain/cecil@e052ab5: Address issue #873 (jbevain/cecil#874) * jbevain/cecil@92f32da: Add `MethodImplAttributes.AggressiveOptimization` (jbevain/cecil#855) * jbevain/cecil@c4cfe16: Fix a race condition between certain Has properties and their collection property. (jbevain/cecil#843) * jbevain/cecil@65a2912: Add more style configuration (jbevain/cecil#854) * jbevain/cecil@9eb00e4: ILProcessor should also update custom debug info (jbevain/cecil#867) * jbevain/cecil@7d36386: Fix corrupted debug header directory entry when writing multiple such entries. (jbevain/cecil#869) * jbevain/cecil@6f94613: InvariantCulture for operand to string conversion in Instruction.ToString() (jbevain/cecil#870) * jbevain/cecil@42b9ef1: Add support for generic attributes (jbevain/cecil#871) * jbevain/cecil@49b1c52: Add `Unmanaged` calling convention (jbevain/cecil#852) * jbevain/cecil@2c68927: Fix mixed module ReadSymbols() (jbevain/cecil#851) * jbevain/cecil@f7b64f7: Fix custom attribute with enum on generic type (jbevain/cecil#827) * jbevain/cecil@79b43e8: Fix deterministic MVID and add PdbChecksum (jbevain/cecil#810) * jbevain/cecil@8b593d5: Harden debug scope update logic (jbevain/cecil#824) * jbevain/cecil@a56b5bd: FieldRVA alignment (jbevain/cecil#817) * jbevain/cecil@75372c7: Switch to netcoreapp3.1 for tests (jbevain/cecil#823) * jbevain/cecil@5f69faa: Add support for generating the method and generic method comment signature with nested types (jbevain/cecil#801) * jbevain/cecil@a0a6ce4: Addressing issue #781 (jbevain/cecil#782) * jbevain/cecil@2f1077d: Update the version of Microsoft.NETFramework.ReferenceAssemblies.net40 (jbevain/cecil#787) * jbevain/cecil@ede17f9: Fix handling of empty string constants (jbevain/cecil#776) #9043 stops building API-34 and makes API-35 stable, and in attempting to do so encounters this error when running all of the in-tree Windows smoke tests on CI: D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-ci.pr.gh9043.6\tools\Xamarin.Android.Bindings.JavaDependencyVerification.targets(22,5): error MSB4062: The "Xamarin.Android.Tasks.GetMicrosoftNuGetPackagesMap" task could not be loaded from the assembly D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-ci.pr.gh9043.6\tools\Xamarin.Android.Build.Tasks.dll. Could not load file or assembly 'Mono.Cecil, Version=0.11.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e'. The system cannot find the file specified. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [D:\a\_work\1\a\TestRelease\07-02_16.40.15\temp\DotNetBuildandroid-armFalseFalseFalse\UnnamedProject.csproj] This appears to be caused by the fact that previously we: 1. Build `Microsoft.Android.Sdk.ILLink.csproj`, which pulls in [Microsoft.NET.ILLink/9.0.0-preview.6.24319.11][0], which pulls in [Microsoft.DotNet.Cecil/0.11.4-alpha.24313.1][1], which contains `Mono.Cecil.dll` versioned as 0.11.5.0. (Yes, it's "odd" that `Microsoft.DotNet.Cecil/0.11.4*` would contain a Cecil versioned as 0.11.5, but that's what it has!) 2. Build the rest of dotnet/android with the [Mono.Cecil/0.11.4][2] package. 3. The Mono.Cecil/0.11.4 package "wins" and ends up in the output directory and the sdk pack. So long as the `ILLink*`-related assemblies don't use any Mono.Cecil/0.11.5 APIs, this works, however it is risky because we don't know exactly what API `ILLink*` uses. #9043 appears to change the build order such that Mono.Cecil/0.11.5 now "wins" and is in the output directory. This causes the above MSB4062 assembly load error. We could try to fix the ordering and make Mono.Cecil/0.11.4 "win", but this leaves us vulnerable to missing some API that `ILLink*` needs. As such, it's better that we update the rest of dotnet/android to use the `0.11.5` version of `Mono.Cecil`. This update breaks the `LinkerTests.FixAbstractMethodsStep_Explicit()` unit test. Specifically, this logic now returns `null` instead of being able to be resolved: new MethodReference (iface_method.Name, void_type, iface) .Resolve (); It feels like this makes sense: a method name and return type doesn't seem like it would be enough to resolve, as parameters are not considered. The fix is simply to use the existing `MethodDefinition` as the `MethodReference`, there is no reason to create a new one. This change fixes the test. [0]: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet9-transport/NuGet/Microsoft.NET.ILLink/overview/9.0.0-preview.7.24328.10 [1]: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet9-transport/NuGet/Microsoft.DotNet.Cecil/overview/0.11.4-alpha.24313.1 [2]: https://www.nuget.org/packages/Mono.Cecil/0.11.4
This is a port of dotnet#31.
Make deterministic MVID unique even when the only change is in PE headers. Implement deterministic PDB ID generation independent of MVID and implement writing of PdbCheckum debug header entry.
Observable differences (other than the above):
GetDebugHeader
(well except for MPDB writer which I didn't change as it's really tricky). Callers should not notice much of a difference if they assumed onlyDispose
would actually write.Module.Write(stream
now requires the stream to be read/write. It already did so for cases where strong name was written, now it requires it also when deterministic MVID is turned on.Deterministic MVID
So far the deterministic MVID was computed as a hash of the IL metadata. It didn't include PE headers.
This means that if there's an assembly which only differs in the PE architecture it would have the same MVID.
This can happen relatively easily for facades (assemblies with only type forwarders) as those are typically
identical across platforms, possibly except for the architecture.
The correct way to calculate MVID is to hash the content of the entire file with the MVID itself and
the strong name signature (if presenet) zeroed out.
This change modified the MVID computation to work that way: Write the entire image with MVID all zeroes,
compute the hash, write the MVID and then compute the strong name.
PdbChecksum
In order for the produced PDB to be fully verifiable the assembly must contain a PdbChecksum
debug header entry which has a cryptographic hash of the associated PDB. The wayt to calculate the checksum
is prescribed as other programs must be able to validate it. See https://github.com/dotnet/runtime/blob/main/docs/design/specs/PE-COFF.md#pdb-checksum-debug-directory-entry-type-19.
This change implements that behavior. Symbols are now written explicitly once all metadata is processed
but before the final assembly image is written. For portable PDBs the full file is written with PDB ID
set to all zeroes. Then the crypto hash is calculated (using SHA256).
The hash is then written into the PdbChecksum debug header entry of the assembly image.
Deterministic Portable PDB ID
So far the PDB ID was set to the same value as MVID. With the fix for MVID described above, this doesn't
work anymore. In case of an embedded PDB, it's not possible to calculate MVID using the hash
with the PDB embedded, as the PDB would now contain the MVID (cycle).
The recommended way to calculate PDB ID is to use the same mechanism as for calculating PDB checksum
and use the first 20 bytes of the has as the PDB ID.
This change implements this by using the first 20 bytes of the hash and sets the PDB ID as the last
change of the portable PDB.
The calculated PDB ID is also then set into the CodeView debug header entry of the assembly.
Added tests for reading and writing both portable and embedded portable PDBs with checksums.
Added test to validate stability of PDB ID across multiple writes.
Added test to validate stability of MVID across multiple writes and the fact that it changes
when just the PE architecture is changed.
This is a fix for dotnet/linker#2203.