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 #9418

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

Better diagnostic for collected delegate #9418

kbaladurin opened this issue Dec 11, 2017 · 9 comments

Comments

@kbaladurin
Copy link
Member

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 (dotnet/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
Copy link
Member Author

cc @Dmitri-Botcharnikov @ayuckhulk

@kbaladurin
Copy link
Member Author

@janvorli @jkotas PTAL

@jkotas
Copy link
Member

jkotas commented Dec 11, 2017

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

@jkotas
Copy link
Member

jkotas commented Dec 11, 2017

cc @noahfalk @lt72

@kbaladurin
Copy link
Member Author

@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
Copy link
Member

jkotas 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 dotnet/coreclr#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
Copy link
Contributor

lt72 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
Copy link
Member Author

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

@jkotas
Copy link
Member

jkotas commented Jan 13, 2018

Fixed by dotnet/coreclr#15809

@jkotas jkotas closed this as completed Jan 13, 2018
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants