Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better diagnostic for collected delegate #15465

Closed
kbaladurin opened this issue Dec 11, 2017 · 9 comments

Comments

@kbaladurin
Copy link
Collaborator

commented Dec 11, 2017

Current version of CoreCLR contains code for supporting MDAs. It is compiled and some parts of it were removed as dead. MDAs are very useful to detect application bugs that related to transitions beetween managed and unmanaged code, for example, to detect calls of collected delegates.

I've tried to enable current implementation of MDAs in coreclr (#15464):

  • I've added xmlparser project to coreclr
  • I've added some previously deleted files (mda.cpp, mdadac.cpp, etc.)

After several fixes CallbackOnCollectedDelegate MDA starts work for me:

$ COMPlus_MDA=1 ./corerun delegategc.dll
Press ESC to stop
register_callback:0x7fe44088316c ----
force GC!!!
upcall ----
A callback was made on a garbage collected delegate of type 'delegategc!DelegateGC.Program+Callback::Invoke'. This may cause application crashes, corruption and data loss. When passing delegates to unmanaged code, they must be kept alive by the managed application until it is guaranteed that they will never be called.

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at System.StubHelpers.StubHelpers.CheckCollectedDelegateMDA(IntPtr pEntryThunk)
   at DelegateGC.Program.upcall()
   at DelegateGC.Program.MyView_KeyEvent(ConsoleKeyInfo keyInfo) in /home/kbaladurin/Desktop/dotnet/coredumps/lldb/dotnet/delegategc/Program.cs:line 65
   at DelegateGC.Program.Main(String[] args) in /home/kbaladurin/Desktop/dotnet/coredumps/lldb/dotnet/delegategc/Program.cs:line 85
Aborted (core dumped)

Is it correct approach to enable MDAs or have you another plans about it?

Thank you!

@kbaladurin

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

@kbaladurin

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

@jkotas

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

What are the MDA you are really interested in? Is the CollectedDelegateMDA the only one?

@jkotas

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

@kbaladurin

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

@jkotas yes, now I'm interested in CollectedDelegateMDA because we often face this problem. But there are other assistants that can be helpful, I think.

@jkotas

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

This diagnostic for calls on collected delegates should be on by default. There is no good reason why it needs to be a special runtime knob. @parjong started working towards it in #12731 by introducing UMEntryThunkCode::Poison. It will detect the calls on collected delegate, but there is no nice error message. It should be pretty straightfoward to make it better by:

  • Changing it to produce nice error message
  • Changing the allocator for UM thunks from LIFO to FIFO so that it takes longer before the thunks get reused.

Doing these two things will make it better than what you get out of the MDA.

@lt72 lt72 added this to the 2.1.0 milestone Dec 11, 2017
@lt72

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2017

@davmason @sywhang: let's pick this up for 2.1

@jkotas jkotas changed the title Enabling MDAs in CoreCLR Better diagnostic for collected delegate Dec 11, 2017
@kbaladurin

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 14, 2017

@jkotas thank you for suggestion! I'll try to use this approach.

@jkotas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

Fixed by #15809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.