Fix data breakpoint issues #46763

sdmaclea · 2021-01-08T19:38:22Z

Currently being tested

Fixes #46644

/cc @chuckries

ghost · 2021-01-08T19:38:26Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Currently being tested

Fixes #46644

/cc @chuckries

Author:	sdmaclea
Assignees:	sdmaclea
Labels:	`area-Diagnostics-coreclr`
Milestone:	6.0.0

src/coreclr/debug/ee/controller.cpp

sdmaclea · 2021-01-08T19:53:38Z

The alternative design would to have TriggerDataBreakpoint return true when we are in SuspendEE and hit a DBP. Then modify this code to not create the DebuggerDataBreakpoint but set DPOSS_USED_WITH_NO_EVENT instead.

runtime/src/coreclr/debug/ee/controller.cpp

Lines 2708 to 2716 in 09a1e77

    
           #ifdef FEATURE_DATABREAKPOINT 
        
               if (stWhat & ST_SINGLE_STEP && 
        
                   tpr != TPR_TRIGGER_ONLY_THIS && 
        
                   DebuggerDataBreakpoint::TriggerDataBreakpoint(thread, context)) 
        
               { 
        
                   DebuggerDataBreakpoint *pDataBreakpoint = new (interopsafe) DebuggerDataBreakpoint(thread); 
        
                   pDcq->dcqEnqueue(pDataBreakpoint, FALSE); 
        
               } 
        
           #endif

I felt this PR might be easier to get through servicing for .NET 5. In my mind it is slightly simpler.

noahfalk · 2021-01-09T00:04:14Z

For patching .NET 5 this looks like a good fix that minimizes the change in behavior from what we have right now. I'm hoping in the longer term fix for .NET 6 we could be a little more aggresive and avoid ever creating/queueing the DebuggerDataBreakpoint controller during the GC. It looks like the current fix is counting on us never going down the JIT write barrier path while the GC is running. Certainly I'd be very surprised if we called the JIT helper from within the GC but it feels more satisfying if long term we didn't have to rely on that kind of assumption.

noahfalk

LGTM!

cshung

Thanks for the fix!

src/coreclr/debug/ee/controller.cpp

sdmaclea · 2021-01-09T02:35:56Z

It looks like the current fix is counting on us never going down the JIT write barrier path while the GC is running. Certainly I'd be very surprised if we called the JIT helper from within the GC but it feels more satisfying if long term we didn't have to rely on that kind of assumption.

As for the JIT write barrier case, we would run the patch until we left the write barrier. Then we would switch to single stepping and hit this code. However, if the GC used managed code, we would stop during GC. It could be problematic.

For patching .NET 5 this looks like a good fix that minimizes the change in behavior from what we have right now. I'm hoping in the longer term fix for .NET 6 we could be a little more aggressive and avoid ever creating/queueing the DebuggerDataBreakpoint controller during the GC.

As I wrote the alternative design description, I wondered whether it might actually be simpler.

I plan to amend this change to

fix deleting pinned handles.
Fix ICorDebugModule4::IsMappedLayout in debug/checked

I am going to draft the more direct approach too. I think it might be simple enough to get through servicing.

sdmaclea · 2021-01-11T21:58:33Z

src/coreclr/debug/ee/controller.cpp

-
-        // Don't queue this object to send an event
-        return false;
-    }


I reverted this change to ignore a DebuggerDataBreakpoint controller when we were in GC. The new change ignores the Data Breakpoint if the data breakpoint is hit during GC suspend (the controller isn't created).

If I left this code, the Data Breakpoint would also be ignored if we entered GC while stepping to find a debug safe point.

So the one risk is somehow we hit a data breakpoint and then enter GC suspend w/o first handling the GC breakpoint.

This comment refers to commit 105d3d4

sdmaclea · 2021-01-12T15:42:40Z

src/coreclr/debug/di/divalue.cpp

-    {
-        event.DisposeHandle.fStrong = FALSE;
-    }
+    event.DisposeHandle.handleType = m_handleType;


@hoyosjs FYI, I should have had more curiosity when you asked about deleting pinned handles. It turns out the implementation layers needed fixes (even though ICorDebug didn't).

sdmaclea · 2021-01-12T15:43:34Z

src/coreclr/debug/di/module.cpp

    {
        hr = pProcess->GetDAC()->IsModuleMapped(m_vmModule, isMapped);
    }
-    PUBLIC_API_END(hr);
+    EX_CATCH_HRESULT(hr);



@hoyosjs This is @chuckries proposed fix to CordbModule::IsMappedLayout

No opposition but it was unclear how this fix relates to the data breakpoints issue? I think we should either clarify how it is connected in comments/commit text or fix it in a separate non-servicing PR.

sdmaclea · 2021-01-12T16:08:36Z

@noahfalk @cshung This changed quite a bit since the last review. Can you take a look again. I plan to take this to servicing.

@chuckries Initial testing for this is complete. Let me know if there is more to do. I would like to be sure this is the last change required before merging and backporting to .NET 5.

sdmaclea · 2021-01-13T19:09:25Z

Code complete for 5.0.3 is Friday. To ask for this fix in 5.0.3, this .NET 6 PR needs to be reviewed and merged.

@noahfalk @cshung @hoyosjs Please (re)review

hoyosjs · 2021-01-13T21:18:23Z

Looks good in terms of the controller logic. As long as Chuck doesn't find any issues with the GC notification being used this looks fine ( I don't think this was the code path - I expect that to have been the out of proc notifications).

As for the other fix, I'm not sure where the InternalApiHolder reference was coming from, so that's a bit surprising... but if that's true the fix looks fine.

noahfalk

This looks good to me. Despite being a bit larger the new factoring is easier to reason about being correct and it looks usable as-is for the .NET 6 fix.

We should clarify how that IsMappedLayout change connects or separate it if it isn't essential. Thanks!

noahfalk · 2021-01-14T00:12:57Z

src/coreclr/debug/di/module.cpp

    {
        hr = pProcess->GetDAC()->IsModuleMapped(m_vmModule, isMapped);
    }
-    PUBLIC_API_END(hr);
+    EX_CATCH_HRESULT(hr);



No opposition but it was unclear how this fix relates to the data breakpoints issue? I think we should either clarify how it is connected in comments/commit text or fix it in a separate non-servicing PR.

sdmaclea · 2021-01-14T00:27:43Z

We should clarify how that IsMappedLayout change connects or separate it if it isn't essential. Thanks!

I'll clarify it in the servicing PR.

It is an orthogonal fix, but w/o it it is impossible to test and verify this PR as VS crashes and restarts because of a debugger assert. While strictly not required for servicing (and may not even make the servicing bar) all testing was done with this patch. The VS team would like the PR as it makes using debug & checked .NET runtime builds possible with VS.

If you really think we shouldn't include this in the servicing PR, I can pull it.

Ignore data breakpoint during GC

327d05b

sdmaclea added the area-Diagnostics-coreclr label Jan 8, 2021

sdmaclea added this to the 6.0.0 milestone Jan 8, 2021

sdmaclea requested review from cshung and noahfalk January 8, 2021 19:38

sdmaclea self-assigned this Jan 8, 2021

sdmaclea commented Jan 8, 2021

View reviewed changes

src/coreclr/debug/ee/controller.cpp Outdated Show resolved Hide resolved

sdmaclea commented Jan 8, 2021

View reviewed changes

src/coreclr/debug/ee/controller.cpp Outdated Show resolved Hide resolved

sdmaclea marked this pull request as draft January 8, 2021 21:26

noahfalk approved these changes Jan 9, 2021

View reviewed changes

cshung approved these changes Jan 9, 2021

View reviewed changes

src/coreclr/debug/ee/controller.cpp Outdated Show resolved Hide resolved

sdmaclea and others added 3 commits January 11, 2021 16:37

Simplify disabling DebuggerDataBreakpoint during GC

105d3d4

Fix destroy pinned handle

55a8d61

Fix CordbModule::IsMappedLayout

5b2800a

sdmaclea commented Jan 11, 2021

View reviewed changes

sdmaclea changed the title ~~Ignore data breakpoint during GC~~ Fix data breakpoint issues Jan 12, 2021

sdmaclea requested a review from hoyosjs January 12, 2021 15:40

sdmaclea commented Jan 12, 2021

View reviewed changes

sdmaclea marked this pull request as ready for review January 12, 2021 15:44

sdmaclea requested review from cshung and noahfalk January 12, 2021 16:04

hoyosjs approved these changes Jan 13, 2021

View reviewed changes

noahfalk approved these changes Jan 14, 2021

View reviewed changes

sdmaclea merged commit 1d1a0f2 into dotnet:master Jan 14, 2021

sdmaclea mentioned this pull request Jan 14, 2021

[release/5.0] Fix data breakpoints #46967

Merged

sdmaclea mentioned this pull request Jan 26, 2021

Fix m_isSuspendedForGarbageCollection #47494

Merged

ghost locked as resolved and limited conversation to collaborators Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data breakpoint issues #46763

Fix data breakpoint issues #46763

sdmaclea commented Jan 8, 2021

ghost commented Jan 8, 2021

sdmaclea commented Jan 8, 2021

noahfalk commented Jan 9, 2021

noahfalk left a comment

cshung left a comment

sdmaclea commented Jan 9, 2021

sdmaclea Jan 11, 2021 •

edited

Loading

sdmaclea Jan 12, 2021

sdmaclea Jan 12, 2021

noahfalk Jan 14, 2021

sdmaclea commented Jan 12, 2021

sdmaclea commented Jan 13, 2021

hoyosjs commented Jan 13, 2021

noahfalk left a comment

noahfalk Jan 14, 2021

sdmaclea commented Jan 14, 2021

Fix data breakpoint issues #46763

Fix data breakpoint issues #46763

Conversation

sdmaclea commented Jan 8, 2021

ghost commented Jan 8, 2021

sdmaclea commented Jan 8, 2021

noahfalk commented Jan 9, 2021

noahfalk left a comment

Choose a reason for hiding this comment

cshung left a comment

Choose a reason for hiding this comment

sdmaclea commented Jan 9, 2021

sdmaclea Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

sdmaclea Jan 12, 2021

Choose a reason for hiding this comment

sdmaclea Jan 12, 2021

Choose a reason for hiding this comment

noahfalk Jan 14, 2021

Choose a reason for hiding this comment

sdmaclea commented Jan 12, 2021

sdmaclea commented Jan 13, 2021

hoyosjs commented Jan 13, 2021

noahfalk left a comment

Choose a reason for hiding this comment

noahfalk Jan 14, 2021

Choose a reason for hiding this comment

sdmaclea commented Jan 14, 2021

sdmaclea Jan 11, 2021 •

edited

Loading