ltrzesniewski / InlineIL.Fody

Inject arbitrary IL code at compile time.
MIT License
238 stars 17 forks source link

InlineIL produces corrupted portable PDB file #12

Closed sakno closed 5 years ago

sakno commented 5 years ago

I trying to build symbol package in Release mode for my project and attach it to the main package. The produced output of dotnet pack -c Release consists of two files: .nupkg and .snupkg. My library uses InlineIL for implementation of several methods. However, NuGet telling me that your PDB file is not portable image If I remove InlineIL.Fody from csproj and leave other weavers then everything is fine. I've tried different combinations but without success.

Workaround: At this moment I decided to use <DebugType>embedded</DebugType> for my project.

ltrzesniewski commented 5 years ago

Interesting... InlineIL needs to modify some stuff in the debug info to get rid of the assembly reference when you use using static InlineIL.IL.Emit; (it also adds sequence points in Debug mode by default), but it's Fody which decides which PDB file format to output, so I'm surprised enabling InlineIL makes a difference. I'd guess the error message is misleading here.

I'll try to reproduce this. Thanks for the report.

sakno commented 5 years ago

The issue is tested on both Linux and Windows. Here is my projects:

  1. csproj file uses InlineIL.Fody and my own weaver. dotnet pack -c Release produces snupkg with invalid PDB file.
  2. csproj uses only my own Fody weaver. This build works perfect.
  3. csproj has InlineIL.Fody weaver without my own weaver. PDB is not valid.

My weaver also modifies IL code and rewrites sequence points. However, it doesn't cause problem with PDB.

Conclusion: the problem is not caused by invalid order of other weavers in csproj file or their intersection.

ltrzesniewski commented 5 years ago

Thanks for the examples. I've tried to build and publish DotNext.Unsafe under a different (and hidden) package id using only dotnet pack -c Release, and I'm getting a different error:

image

This error makes more sense than "not a portable PDB". I'll try to figure out why the checksum doesn't match.

In the meantime, could you tell me if the PDB in the snupkg file you tried to upload starts with BSJB (as the first 4 bytes)? Because apparently that's the check the NuGet server is doing (see here).

ltrzesniewski commented 5 years ago

So it looks like that Cecil currently doesn't handle PDB checksums. Here's the spec and here's the list of entries Cecil knows about, and as you can see there's no entry type 19 in there. 😞

The dll that Fody outputs doesn't even have a PDB checksum entry.

Here's the debug directory of the project when built with Fody disabled:

image

And here's what happens when Fody is enabled:

image

Even if I run Fody without any weaver, I get the same thing as above: there's no PdbChecksum entry.

So... Are you sure you managed to upload a snupkg file that was produced by using Fody with your own weaver? I checked DotNext.Reflection v0.12.0 and it doesn't have a PdbChecksum entry either. NuGet.org should show the following links for a package with an associated symbol package:

image

But it doesn't show the "Download symbols" link for DotNext.Reflection.

sakno commented 5 years ago

Yep, you right. Reflection didn't have Symbols package. Probably it was dropped by Nuget without error. It's sad because a bug somewhere inside of Cecil. However, I checked today the checksum of embedded PDF and was fine. Look at this code in Fody repo. It uses different provider for embedded PDB which is probably working correctly. So my workaround is the only possible way to publish debug information. Anyway, many thanks for investigation!

sakno commented 5 years ago

Did you use dotPeek for analysis? I'll double check the correctness of embedded PDB tomorrow, don't have suitable tool for that on Linux.

ltrzesniewski commented 5 years ago

You're welcome! 😉

I wouldn't really call this a bug in Cecil, as the checksum stuff is quite recent... It just was never implemented. I even didn't find any issue in the Cecil repo which mentions it, but I guess if the snupkg file format becomes popular this may become a problem and someone will implement it.

Embedded PDBs are very similar to portable PDBs, they're just deflated portable PDBs that are embedded in the assembly. EmbeddedPortablePdbWriter is a wrapper around PortablePdbWriter, see here).

TBH I don't like that snupkg stuff anyway, as embedded PDBs work just fine and without requiring the user to configure anything.

As for the analysis, I first used this code from the NuGet server, but when it showed that there's no PdbChecksum entry at all, I opened the files in dotPeek just to make sure. dotPeek has a very nice metadata viewer which I found useful for this kind of stuff more than once.

SimonCropp commented 5 years ago

my perspective. i always use embedded. the reason being is, when people raise issues against my projects with a stack trace, i want the line numbers without any extra dev config for them and without them needing to remember to deploy pdbs to production.

sakno commented 5 years ago

I'm switched to embedded in the last release. But what if NuGet decides to verify checksum of the embedded PDF?

ltrzesniewski commented 5 years ago

To be honest I don't know why Roslyn even bothers to emit the checksum for embedded PDBs... So yes I suppose NuGet could decide to validate this field, but I don't see a good reason to do that given it's in the same file.

Anyway, I reported an issue in the Cecil repo, and I may even give a shot at implementing this someday. I guess that if NuGet starts enforcing the checksum then this issue will get resolved really fast. 😉

ltrzesniewski commented 5 years ago

I'm closing this as it's an upstream issue. See https://github.com/jbevain/cecil/issues/610 for further info.