Samsung / netcoredbg

NetCoreDbg is a managed code debugger with MI interface for CoreCLR.
MIT License
800 stars 103 forks source link

exception-breakpoints: allow filtering out 'internal' excetpions #126

Open noam-sol opened 1 year ago

noam-sol commented 1 year ago

Hey, We've noticed that when 'all' is selected from ['all', 'user-unhandled'] in vscode exceptions, then it also breaks on exceptions which their entire path ('throw' to the handler) is internal to libs, which lead to some noise when debugging.

I'd like to add the ability to filter those out, probably by calling EnableExceptionCallbacksOutsideOfMyCode(false). Should it be default in vscode's "all", or maybe a new option? I'm not sure. Any ideas?

viewizard commented 1 year ago
  1. If I remember correct, we don't implement this because of issues with thread status tracking for some cases and this need more time for investigation. Imho, if you can't guarantee that you will care about all around, best solution will be keep proper tracking but don't emit stop event in this case for 2 cases covered by EnableExceptionCallbacksOutsideOfMyCode(false) (DEBUG_EXCEPTION_FIRST_CHANCE and DEBUG_EXCEPTION_FIRST_CHANCE -> DEBUG_EXCEPTION_CATCH_HANDLER_FOUND) instead of EnableExceptionCallbacksOutsideOfMyCode(false) usage itself.
  2. Runtume could have second internal unhandled exception during work with exception (out of memory, for example), that will mean debugger will never know initial exception and/or related to it data: should be (normal): DEBUG_EXCEPTION_FIRST_CHANCE -> DEBUG_EXCEPTION_USER_FIRST_CHANCE -> DEBUG_EXCEPTION_CATCH_HANDLER_FOUND will be (out of memory in runtime): DEBUG_EXCEPTION_FIRST_CHANCE -> DEBUG_EXCEPTION_UNHANDLED will be (out of memory in runtime) + EnableExceptionCallbacksOutsideOfMyCode(false) : DEBUG_EXCEPTION_UNHANDLED
  3. This should work with JMC enabled by user only (since debuggers use JMC internally all the time for stepping and more).

Should it be default in vscode's "all", or maybe a new option?

We keep same default behaviour as MS debuggers. Is vsdbg behaviour was changed? Probably, since this should work for all protocols, make it with env, that ExceptionBreakpoints class constructor read and care about logic.

noam-sol commented 1 year ago

issues with thread status tracking

@viewizard can you share some details about what could go wrong? I'm starting to get into the codebase and I'd love to hear about all these edge cases. Thanks!

I mean, each exception starts with FIRST_CHANCE, so the thread state would be reset for any time a relevant exception is thrown, right?

viewizard commented 1 year ago

can you share some details about what could go wrong?

That was about 3 years ago when I refactor exception breakpoints, and this case was not even analyzed carefully (since we decided to skip it from implementation at early stage). Unfortunately, I have nothing to add to my previous post now.

I mean, each exception starts with FIRST_CHANCE, so the thread state would be reset for any time a relevant exception is thrown, right?

This is not so simple. :-) See my comment with runtime callback calls logic and current implemented debugger logic: https://github.com/Samsung/netcoredbg/blob/db69338cf1606d8d327de0eaaf1694d8463af135/src/debugger/breakpoints_exception.cpp#L329-L360 In short: exception could start with DEBUG_EXCEPTION_FIRST_CHANCE, DEBUG_EXCEPTION_USER_FIRST_CHANCE or DEBUG_EXCEPTION_UNHANDLED. Only in case first (start) exception callback call you could get initial exception related data (sure, this also could be done with evaluation of exception managed object if... if you able to evaluate(execute managed code of property), that could be not possilbe in case of internal runtime fail or some GC related state that will prevent evaluation on thread with exception).