pdb round trip loses iterator information about cecil HOT 31 CLOSED

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

from cecil.

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...

from cecil.

Here's the diff of my branch:

http://gist.github.com/377904

As I said, it's half working.

from cecil.

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

from cecil.

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?

from cecil.

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

from cecil.

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

from cecil.

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.

from cecil.

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?

from cecil.

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

from cecil.

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

from cecil.

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?

from cecil.

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

from cecil.

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!

from cecil.

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).

from cecil.

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

from cecil.

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 ?

from cecil.

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?

from cecil.

@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?

from cecil.

@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!

from cecil.

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 ;)

from cecil.

See, cool :)

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

from cecil.

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

from cecil.

@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

from cecil.

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

from cecil.

@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 :)

from cecil.

This one can be closed.

from cecil.

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

from cecil.

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

What's the issue with async/await?

from cecil.

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

from cecil.

Fixed in #353.

from cecil.