ArgumentNullException reading PDB symbols

[spouliot]

Extracted from issue https://github.com/xamarin/xamarin-macios/issues/12306

The exception occurs when processing System.Void Abp.Auditing.AuditingInterceptor/<InternalInterceptAsynchronous>d__61::MoveNext()`

This is not something the SDK can "catch-and-continue".

It's possible this is a corrupted PDB but it happened on more than one version (6.4 and 6.3.1) of the ABP package.

It also happens without Xamarin.iOS using Cecil 0.11.4 (have not tried any older version, or main branch).

An unhandled exception of type 'System.ArgumentNullException' occurred in Mono.Cecil.dll: 'Value cannot be null.'
   at Mono.Cecil.Cil.SequencePoint..ctor(Int32 offset, Document document)
   at Mono.Cecil.SignatureReader.ReadSequencePoints(Document document)
   at Mono.Cecil.MetadataReader.ReadSequencePoints(MethodDefinition method)
   at Mono.Cecil.Cil.PortablePdbReader.ReadSequencePoints(MethodDebugInformation method_info)
   at Mono.Cecil.Cil.PortablePdbReader.Read(MethodDefinition method)
   at Mono.Cecil.Cil.CodeReader.ReadMethodBody()
   at Mono.Cecil.Cil.CodeReader.ReadMethodBody(MethodDefinition method)
   at Mono.Cecil.MetadataReader.ReadMethodBody(MethodDefinition method)
   at Mono.Cecil.MethodDefinition.<>c.<get_Body>b__41_0(MethodDefinition method, MetadataReader reader)
   at Mono.Cecil.ModuleDefinition.Read[TItem,TRet](TRet& variable, TItem item, Func`3 read)
   at Mono.Cecil.MethodDefinition.get_Body()
   at <Program>$.<<Main>$>g__Process|0_0(TypeDefinition type) in /Users/poupou/git/spouliot/dotnet-tools/cecil-pdb-check/Program.cs:line 29
   at <Program>$.<<Main>$>g__Process|0_0(TypeDefinition type) in /Users/poupou/git/spouliot/dotnet-tools/cecil-pdb-check/Program.cs:line 25
   at <Program>$.<Main>$(String[] args) in /Users/poupou/git/spouliot/dotnet-tools/cecil-pdb-check/Program.cs:line 17

Abp.zip

[jbevain]

Thanks for filing this and for the repro, I'll have a look pronto. Merci!

[spouliot]

https://github.com/spouliot/dotnet-tools/tree/master/cecil-pdb-check for the code I used to verify

FWIW it seems to be the only method, that has a problem, inside the assembly.

[jbevain]

@spouliot it looks like ILSpy / System.Reflection.Metadata is throwing on this as well, and the sequence points are not properly encoded (the document information is missing).

I'm looking at how Cecil could handle that and maybe deal with a null doc, but it looks like whatever created this portable pdb information did not create this right. Is this straight from csc? Has this been processed by Cecil and not generated correctly, that would be fun.

[jbevain]

A quick fix would look like:

diff --git a/Mono.Cecil.Cil/SequencePoint.cs b/Mono.Cecil.Cil/SequencePoint.cs
index 725d307..c97e288 100644
--- a/Mono.Cecil.Cil/SequencePoint.cs
+++ b/Mono.Cecil.Cil/SequencePoint.cs
@@ -57,9 +57,6 @@ namespace Mono.Cecil.Cil {

        internal SequencePoint (int offset, Document document)
        {
-           if (document == null)
-               throw new ArgumentNullException ("document");
-
            this.offset = new InstructionOffset (offset);
            this.document = document;
        }
diff --git a/Mono.Cecil/AssemblyWriter.cs b/Mono.Cecil/AssemblyWriter.cs
index 3f17a82..12e0ab3 100644
--- a/Mono.Cecil/AssemblyWriter.cs
+++ b/Mono.Cecil/AssemblyWriter.cs
@@ -2548,6 +2548,9 @@ namespace Mono.Cecil {

        public MetadataToken GetDocumentToken (Document document)
        {
+           if (document == null)
+               return MetadataToken.Zero;
+
            MetadataToken token;
            if (document_map.TryGetValue (document.Url, out token))
                return token;

But we have another alternative, which is to reuse the original document associated to the method and skip null document entries in the sequence points. It's not respectful of the portable pdb spec though, but I feel it's more useful to the user.

Would love to know how this portable pdb was generated before taking a decision between the two options.

[spouliot]

Is this straight from csc? Has this been processed by Cecil and not generated correctly, that would be fun.

The assembly comes from https://github.com/xamarin/xamarin-macios/issues/12306 (from a code consumer, not the code publisher) and is available from nuget.org

Both the mentioned version and the more recent (next) one fail identically, so it does not look like something randomly corrupt was published. However I'm not sure how the original packages were produced...

Would love to know how this portable pdb was generated before taking a decision between the two options.

I have mixed emotions here. Solving the issue by hiding sounds like something that will come back to bit us in the future.

In an ideal world we would be able to report [1][2] something back to the developer, since it might impact the debuggability of his application.

[1] There's no such mechanism in Cecil and adding (a good) one would likely not be trivial. [2] For testing purpose this could continue to throw an exception

[Zastai]

I figure accepting anything that Roslyn (i.e System.Reflection.Metadata) does not is not necessary. Such a PDB will already prevent debugging in Visual Studio (which I suppose could be the point - but then why ship a PDB at all).

If it can be done with no immediately ill effects (other than potentially not recognizing corrupted data), like not throwing when the sequence point table is not in strict ascending order of offset, that's fine (although some diagnostic for acceptable-but-not-following-the-spec situations would still be useful).

But in general, I would much rather get a useful exception, like "entry #5 in sequence point table (RID nnnn) has nil document". Otherwise things can get quite vexing (like the somewhat misleading "Invalid compressed integer" exception reported by Roslyn in #840).

[spouliot]

https://github.com/icsharpcode/ILSpy/issues/2823 , while a different issue (and stack trace) that happens after ILSpy reprocessed the .pdb file, leads to the same issue.

A bit tragic considering that the app is not actually linked (but reuse the linker pipeline) so the PDB should not even be loaded...