Facade assembly symbol indexing collision due to illinker #45649
Comments
Dotnet-GitSync-Bot
added
the
untriaged
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @tommcdon Issue DetailsDescriptionThere are no symbols for System.Runtime.dll on Microsoft Symbols Server:
|
|
@mmitche fyi |
|
Using dotnet-symbol I can download both the Windows and Portable PDBs for the 5.0.0 System.Runtime.dll. I'm not sure why symsrv/symchk you are using doesn't find it. Can you send me the exact command line you are using? |
|
Looks like the reason is they are doing IL-only PDB fetching. We don't index those |
|
@mikem8361 I use Visual Studio with Microsoft symbols server. |
|
@mikem8361 the image has two debug directories: We index the R2R pdb, but not the IL-Only one. |
|
Dotnet-symbol downloads the IL one just fine with A11BAEED-4C47-491B-80F6-FB479B20A1E9.
|
|
Looks like I had it wrong. The IL PDB is indexed as
The R2R PDB is indexed as only under the full key:
However, the check seems to fail. I can repro this locally. Will continue taking a look. |
|
Looking at @NN--- 's trace, this looks the problem: If I am reading that correctly, that means the portable IL PDB is being downloaded (good), but the symbol server is returning something which is either bogus, or which symsrv.dll doesn't understand, as 820 bytes is clearly not enough for a PDB. @hoyosjs I think your problem could be different from what @NN--- saw. Are you seeing a case where the wrong PDB is downloaded? @NN--- Do you still have this problem? If so, could you tell us more what 'c:\symbols\System.Runtime.pdb\A11BAEED4C47491B80F6FB479B20A1E9ffffffff\System.Runtime.pdb' looks like --
|
|
The PDB for System.Runtime should be pretty small. I verified the two PDBs do exist and the portable version is indeed (Both symweb and msdl) and that they are 820 bytes. (My local build has one with 696 bytes). It also has the classical portable "BSJB" set of first four bytes. I talked a bit with Chuck, and he confirmed what I thought: the SymReader for portable pdbs is saying things don't match. I'll do some testing on top of this removing VS from the equation.
Yeah, mine ended up being actually not getting the metadata for creating the key at all. Ironically the only problematic DLL is the one this is System.Runtime. WinDBG somehow reconstructs the key. I might dig a bit more some other day. |
|
Seems like all the ones that fail are the type forwarding assemblies. Although some stuff doesn't add up - if I fetch the symbols from storage something wrong, which hints at a bug in the uploading process. This is the full list that will fail verification
|
hoyosjs
changed the title
hoyosjs
changed the title
hoyosjs
added
area-Meta
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF Issue DetailsDescriptionThere are no symbols for System.Runtime.dll on Microsoft Symbols Server:
|
|
It looks like this is the linker that is causing this. If I build a facade - say
Examplex64 pre linker x64 after linking x86 before linking x86 Before Linking x86 after linking This doesn't happen seem to happen with non-facade assemblies in the shared framework (at least on x64 windows where I verified). Currently there's no major break in the Watson-like scenarios, or dump debugging. However, I still don't fully know what causes the GUID collisions, so if this were ever to happen in non-facade IL assemblies and their PDBs, then we'd have a bigger issue. So far it looks like for 5.0.0 - 5.0.2 facades for coreclr are indexed under the same guid and all facades for mono builds are under a separate guid if the two runtimes share . (For example System.Runtime forwards to System.Private.CoreLib, so there's one guid that gets used for coreclr and another for mono. On the other hand, System.XML.XPath forwards to System.Private.XML, which is largely shared leading to just using one GUID but three different timestamps). |
|
I would think that facades don't run through the linker - there's nothing to do on them anyway. Would be interesting to know why this happens (if it actually does). As for the GUIDs being the same before and after linking - linker implemented deterministic output, which includes copying the MVID from the input to the output. If we use linker in the deterministic mode, then this behavior is currently by design. But maybe we need to change that behavior. |
|
@vitek-karas Does the linker change anything in the PDB? If not then both timestamp and guid in the CodeView entry should remain the same. If it does then either
|
|
Linker will roundtrip the PDB if it processes an assembly (it has to, if it's making changes to the assembly). The interesting bit here is that running linker on facades doesn't make sense, so we should not do it. |
|
@vitek-karas two quick questions:
|
|
Got me a while to make sure I understand what's going on. There are basically two problems here:
It seems this should be a problem basically for any assembly which has exactly the same content across architectures. |
ViktorHofer
changed the title
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsDescriptionThere are no symbols for System.Runtime.dll on Microsoft Symbols Server:
|
|
@vitek-karas should we keep this in 6.0 or move it out? Asking as you just recently filed an issue for it with the milestone set to 6.0: dotnet/linker#2203. |
|
I spent some more time looking into this. While the problem exists in all versions of the linker/.NET, this seems to be primarily .NET 5 problem. In .NET 5 the managed assemblies were built separately for x86 and x64. So for example the .NET 5 On .NET 6+ the x86 and x64 builds of Also the symbol indexing seems to work for 6.0, debugging a simple app in VS correctly loads symbols for all assemblies (including |
|
Hi @vitek-karas. Did you identify any problems that we should fix? It sounds like you were able to confirm that 6.0 binaries in the runtime packs matched what's on the symbol server. |
|
There are definitely two separate issues in the linker/cecil. It doesn't correctly consider all of the assembly when computing a hash (from which it derives the deterministic MVID) - and I was able to repro the problem on a simple example easily (assemblies which only differ by architecture in the PE header will get the same MVID after trimming -> bad). And second it doesn't write the PdbChecksum entry into the output at all. This is a bit less important, but still would be really good to fix. What I mentioned above is that it seems that these has much lower impact on .NET 6 than on .NET 5, but they still exist and should be fixed. |
We should fix this too since Xamarin Android uses the MVID to deduplicate architecture-independent assemblies so we definitely want to make sure both builds are identical. |
|
Just update on what I found after digging into this for real. Issue 1 - MVID is not 100% uniqueThis is the issue mentioned above. Cecil doesn't include the PE file headers into the MVID hash computation, it only uses data from the IL Metadata tables and blobs. This leads to facades having collisions on MVID as the IL side of the dll is identical. This should be fixed by hashing the entire dll with MVID all zeroed and then just patching the MVID from the computed hash as the last thing to be done on the dll (well except where there is strong name signing, but I don't think we use this in the framework). Issue 2 - Cecil reuses MVID as PDB IDCecil uses the MVID in the PDB as the PDB ID. This is actually very problematic for the fix for Issue 1, especially if there are embedded PDBs. On its own this is not something which should necessarily break anything. But it makes the correct implementation basically impossible, so this will need to be changed. Issue 3 - Cecil doesn't write PdbChecksum debug header entryThis causes NuGet to raise warnings (potentially) and should be fixed. This is only applicable to portable PDBs (Cecil also supports Windows Native PDBs and also MDB - mono symbols format), so the changes are not as big, unfortunately this will require changing the order in which things are done in Cecil -> potentially relatively risky change. |
|
The fix for this has been merged into mono/cecil and also into dotnet/linker/main. So this should be fixed in .NET 7 for sure. |
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)
|
@vitek-karas should we move this to 8.0? |
|
I'm moving it to 6.0 actually, because this is now only tracking whether we take a backport to 6.0. |
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue DetailsDescriptionThere are no symbols for System.Runtime.dll on Microsoft Symbols Server:
|
ViktorHofer
added
area-Tools-ILLink
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsDescriptionThere are no symbols for System.Runtime.dll on Microsoft Symbols Server:
|
Description
There are no symbols for System.Runtime.dll on Microsoft Symbols Server:
The text was updated successfully, but these errors were encountered: