pdb round trip loses iterator information #4

Open
jbevain opened this Issue Apr 19, 2010 · 30 comments

Projects

None yet

6 participants

@jbevain
Owner
jbevain commented Apr 19, 2010

When round tripping a pdb with Mono.Cecil and Mono.Cecil.Pdb, we're losing some custom data about iterators. VS is then unable to debug properly iterator variables.

It looks fixable.

With the CCI code, a PdbFunction has a iteratorClass field, mapping a method to its iterator class. Then the MoveNext method has an iteratorScopes field, which is a collection of scopes.

To encode that back in the PdbWriter, we have to use SetSymAttribute in between OpenMethod and CloseMethod. The PeWriter file of the CCI has the information.

@jbevain
Owner
jbevain commented Apr 23, 2010

I have a half working local branch, but it involves a lot of annoying changes. Am not sure the pain is worth the gain.

@Rageous
Rageous commented Apr 24, 2010

Can you provide a patch at least for me?
I'm extremely interested in this feature since I has a few large projects with lots of "yieldy" code that becomes undebuggable after patching...

@jbevain
Owner
jbevain commented Apr 24, 2010

Here's the diff of my branch:

http://gist.github.com/377904

As I said, it's half working.

@Rageous
Rageous commented Apr 25, 2010

Thanks!
What's in the not-working half? :)

@Rageous
Rageous commented Apr 25, 2010

There is a problem with this patch. It looks like Mono.Cecil.Cil.CodeWriter.WriteMethodBody method writes unresolved "yieldy" method incorrectly. Though if I resolve it, it's written ok. I've tried to fix WriteUnresolvedMethodBody method disabling "symbols.Offsets" check, but failed: PdbWriter crushes trying to write missing symbols offsets.

Can you give me an advice, how should I better fix this problem: by resolving all methods (could be slow I think?) or by less naive patching of WriteUnresolvedMethodBody method?

@Rageous
Rageous commented Apr 25, 2010

And by the way, I've found not-working half: debugger doesn't see any variables below "yield return ..."

@Rageous
Rageous commented Apr 25, 2010

I've fixed variables count related bug: http://gist.github.com/378395

@atykhyy
atykhyy commented May 26, 2011

Based on both these patches I made one that works for me: atykhyy@a871bef
It handles both resolved and unresolved methods correctly. I can step inside iterators and see correctly scoped iterator locals, although I also see compiler's '$CS$nnnn' locals as in all other methods.

@Rageous
Rageous commented May 26, 2011

Wow, that's really great! I'll use it for sure - since my patch is obsolete and rebasing it onto head revision of Cecil is rather complicated, new one is far more useful.
May be now authors will include this fix into trunk?

@jbevain
Owner
jbevain commented May 26, 2011

Now that's pretty cool. I'll see about integrating this at some points. Thanks!

@Rageous
Rageous commented Jul 18, 2011

A small fix for the one above: https://gist.github.com/1089405 -- to avoid null reference exceptions in some rare cases.

@davidroth

We are fighting with lost debugging because of async/await and cecil: https://github.com/SimonCropp/NotifyPropertyWeaver/issues/15#issuecomment-10987709

Is there some hope for a fix anytime soon?

@distantcam

So what's the deal with this? Given it's been open 3 years is there any hope it will be patched?

@davidroth

Please @jbevain give us a short statement if you plan to fix this any time soon or not.
This is a massive productivity problem when using lots of async code with the c# async keyword, so your statement would help me to decide if its worth to live with it for now (and wait for a fixed cecil), or to update our project to use Postsharp (if you say it won`t be fixed for the next xxx Months/Years etc.)
Thanks!

@jbevain
Owner
jbevain commented Mar 12, 2013

Hey guys,

Sorry to read that this bug is annoying you. Blame the obnoxiousness and closeness of the pdb format.

The quick fix is easy, rebase the patches from atykhyy@a871bef on top of master and be done with it.

A proper fix with a proper API would take a bit longer to implement.

It's hard to give an estimate as I'm currently super busy bootstrapping my company and working on our product.

That being said if you want expedite things as this is business matter for you, you can contract my company to implement that, and we'll slide that in between two missions in early April (best we can do right now).

@distantcam

So you won't accept the current patch? That's a pity.

@SimonCropp

unfortunately the patch from atykhyy@a871bef does not merge cleanly with the current head. I gave it a go but think my limited knowledge of git was getting in the way.

I suspect that this is one of the reasons @jbevain has not "accepted the patch". It is not a trivial thing.

Happy to be proven wrong on this account.. Does someone want to have a go at this merge? If it can be cleanly done then point me to the repo and I could do a custom build and release of Cecil (after running it through a battery of tests).

Thoughts @distantcam @davidroth ?

@distantcam

I am more than happy to put the work into getting this patch working in the current branch. But the issue then is would it be accepted since it's been sitting here for 3 years?

@jbevain
Owner
jbevain commented Mar 13, 2013

@distantcam this patch:

  • Augments the memory usage of Cecil,
  • Adds new APIs I don't like,
  • Doesn't respect the coding conventions,

So no, it won't be merged in master, given the fact that anyone with git and minimal knowledge of C# can take the two fixes and cherry-pick them on top of master to use in a branch until this is fixed properly. That's how we've been adding large features to Cecil, like thread safety and lower memory usage.

How's that a pity?

@davidroth

@jbevain Thanks for your comments, did not really know that the 3 year old patch is an option here, but I`ll give it a try now!

@SimonCropp @distantcam I will do the rebase onto master today and will hopefully publish it today (if it's working). I`ll let you know!

@davidroth

Ok guys, I cherry-picked the changes and re-applied it onto master. It is in my pdbiteratorloss branch which you can find here: davidroth@9609b34

First, good news @SimonCropp and @distantcam: In a quick test I replaced all the mono cecil assemblies in Fody.PropertyWeaver and made a fresh build of our project => There were no errors and debugging async methods works like a charm now - so it looks like the merge is ok!

Second, disclaimer: There were some annoying conflicts in PdbReader.cs and PdbWriter.cs which i managed to resolve, but i must admit that I have no real experience with cecil or the pdb format itself so don`t blame me if there are any issues ;)

@jbevain
Owner
jbevain commented Mar 13, 2013

See, cool :)

I can even use that to fix the points that bug me in this patch more easily.

@jbevain
Owner
jbevain commented Mar 13, 2013

@SimonCropp can you run your tests with the symbols branch on jbevain/cecil?

@SimonCropp

@jbevain yes that fixes debugging variables in iterators (at least for my simple test case).

I also ran it through 1000+ unit tests and it passed all of them

@SimonCropp

@davidroth @distantcam do either of you want an internal build of Fody with the new version of Cecil?

@davidroth

@SimonCropp thx, but I have already patched our version, so no-pre build necessary at least for me. However, it would be nice if new "official" Fody releases would include these changes :)

@SimonCropp

This one can be closed.

@SimonCropp

To be specific the enumerator bug is fixed but a new bug should be raised for async await

@jbevain
Owner
jbevain commented Apr 26, 2013

@SimonCropp I'm still discussing this with the CCI/PDB guys.

What's the issue with async/await?

@davidroth

@SimonCropp I dont have any issues with async await. I am using the patched version in my pdbiteratorloss branch (see earlier comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment