This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Process/NetBSD] Fix handling concurrent watchpoint events
ClosedPublic

Authored by mgorny on Nov 8 2019, 11:57 AM.

Details

Summary

Fix handling concurrent watchpoint events so that they are reported
correctly in LLDB.

If multiple watchpoints are hit concurrently, the NetBSD kernel reports
them as series of SIGTRAPs with a thread specified, and the debugger
investigates DR6 in order to establish which watchpoint was hit. This
is normally fine.

However, LLDB disables and reenables the watchpoint on all threads after
each hit, which results in the hit status from DR6 being wiped.
As a result, it can't establish which watchpoint was hit in successive
SIGTRAP processing.

In order to workaround this problem, clear DR6 only if the breakpoint
is overwritten with a new one. More specifically, move cleaning DR6
from ClearHardwareWatchpoint() to SetHardwareWatchpointWithIndex(),
and do that only if the newly requested watchpoint is different
from the one being set previously. This ensures that the disable-enable
logic of LLDB does not clear watchpoint hit status for the remaining
threads.

This also involves refactoring of watchpoint logic. With the old logic,
clearing watchpoint involved wiping dr6 & dr7, and setting it setting
dr{0..3} & dr7. With the new logic, only enable bit is cleared
from dr7, and the remaining bits are cleared/overwritten while setting
new watchpoint.

Diff Detail

Event Timeline

krytarowski added inline comments.Nov 8 2019, 1:47 PM
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
317

I presume that in this code path we land into a scenario that:

  1. Trap on a different LWP
  2. User sets new watchpoints
  3. We land here with a SIGTRAP on old watchpoint that was wiped out.

If so, we shall ignore this report, bail out and resume execution with PT_CONTINUE.

I think that this path could be some remnant from Linux shared trap reasons.

mgorny marked an inline comment as done.Nov 9 2019, 2:59 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
317

I was wondering whether it's better to silently ignore unknown watchpoints or stop the process. Decided the latter is better for the user, in case it carried some useful information still. Not to mention it has lower risk of crashing lldb-server if I get some logic wrong.

mgorny marked an inline comment as done.Nov 9 2019, 3:02 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
892

Just occurred to me I can probably reuse the new function here.

mgorny updated this revision to Diff 228568.Nov 9 2019, 4:01 AM

Updated as noted in comments, particularly reuse ClearWatchpointHit() when setting new wp.

mgorny updated this revision to Diff 228955.Nov 12 2019, 1:41 PM

Included fixed tests.

labath accepted this revision.Nov 25 2019, 6:55 AM

LGTM, since this seems to be the best we can do given the current netbsd behavior.

However, I'd like to repeat what I said on the IRC, that I consider this behavior of netbsd to be unreasonable. Serializing signals delivery seems nice at a first glance, but when those signals reflect events which are "trapped" (== caught only after the relevant thing has happened), as is the case with watchpoints on x86, things just start getting weird. This means that if we get a "concurrent" watchpoint hits, we will only report the one of the watchpoint hits, but the entire state of the program will be already in the "post-hit" state for both watchpoints (the memory will contain the new value, PC will point after the trapping instruction, etc.). I think this state can be very confusing to the user, particularly if he decides to run an expression in this state (to figure out what happened?) and then this expression is interrupted by the delayed delivery of the second watchpoint signal.

This revision is now accepted and ready to land.Nov 25 2019, 6:55 AM

LGTM, since this seems to be the best we can do given the current netbsd behavior.

However, I'd like to repeat what I said on the IRC, that I consider this behavior of netbsd to be unreasonable.

Thanks for the feedback. Evaluating the options, we will keep using our current model. There are no plans for any modifications in this domain.

The end user of a debugger is just expected to see no difference.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 11:14 AM