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

Cleanup MDAs stale code #12005

Closed
stephentoub opened this issue Feb 8, 2019 · 9 comments
Closed

Cleanup MDAs stale code #12005

stephentoub opened this issue Feb 8, 2019 · 9 comments
Assignees
Milestone

Comments

@stephentoub
Copy link
Member

The runtime still has code related to MDAs, e.g.
https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/mda.cs

Is there a plan for these moving forward? Should some of them, e.g. ReportErrorSafeHandleRelease/FireInvalidGCHandleCookieProbe/etc., be turned into EventSource events and the rest deleted?

Interop issue: https://github.com/dotnet/coreclr/issues/22538

@danmoseley
Copy link
Member

Don't MDA's cause synchronous debugger breaks - presumably I can't achieve that with EventSource events?

@stephentoub
Copy link
Member Author

Don't MDA's cause synchronous debugger breaks

On coreclr? How do you turn them on?

@AaronRobinsonMSFT
Copy link
Member

Based on some casual conversations I have had with @jkotas, I don't believe that MDAs are planned on being supported in coreclr. My conversations have been around the ones involving Interop and stack validation.

@danmoseley
Copy link
Member

On coreclr? How do you turn them on?

I meant on regular CLR - meaning that an asynchronous event would not match the functionality. Having said that I almost never used them myself.

@jkotas
Copy link
Member

jkotas commented Feb 9, 2019

The MDAs are have been a weird animal and generally not very useful. We have not added any new MDAs for ~10 years. I do not expect we will add them back to CoreCLR.

Some/most of them can be just deleted, some of them turned into logging, some of them into dianostic tools that can be turned on as needed.

ReportErrorSafeHandleRelease

Delete. There is thrown and ignored exception already. I think it is good enough to give you a clue that there may be something wrong.

FireInvalidGCHandleCookieProbe

Delete. If we wanted to have better diagnostic for use-after-free for GC handles, it should be built into the GC handle manager itself. The GCHandle cookies MDA was implemented in non-pay-for-play way. It made regular access to GC handles signficantly slower even when the MDA was turned off.

@jkotas
Copy link
Member

jkotas commented Feb 9, 2019

The whole mda.cs can be deleted, I think.

@tommcdon
Copy link
Member

Closing as it looks like MDA code was removed per dotnet/coreclr#22535

@stephentoub
Copy link
Member Author

@tommcdon, there's still other MDA code in various places in the runtime, but the managed mda.cs file for MDAs fired from Corelib was deleted in that PR.

@tommcdon
Copy link
Member

Thanks @stephentoub! Reactivating the issue.

@tommcdon tommcdon reopened this Feb 26, 2019
@hoyosjs hoyosjs changed the title Status of MDAs? Cleanup MDAs stale code Feb 26, 2019
@jkotas jkotas closed this as completed May 23, 2019
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 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

7 participants