Page MenuHomePhabricator

[lldb] [Process/FreeBSDRemote] Explicitly copy dbregs to new threads
ClosedPublic

Authored by mgorny on Nov 8 2020, 8:03 AM.

Details

Summary

Explicitly copy dbregs to new threads to ensure that watchpoints
are propagated properly. Fixes the test failure due to apparent kernel
race between reporting a new thread and resuming main thread execution
that makes implicit inheritance of dbregs unreliable. By copying them
explicitly, we ensure that the new thread correctly respects watchpoints
that were set after the thread was created but before it was reported.

The code is copied from the NetBSD plugin and modernized to use
llvm::Error.

Diff Detail

Event Timeline

mgorny requested review of this revision.Nov 8 2020, 8:03 AM
mgorny created this revision.
mgorny added a comment.Nov 8 2020, 2:43 PM

Alternatively, we could relist threads on every stop.

labath accepted this revision.Nov 9 2020, 12:48 AM

So, is this like a kernel bug that we're working around here? Would it make sense to file a bug on the freebsd tracker and reference it from here?

This revision is now accepted and ready to land.Nov 9 2020, 12:48 AM

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250954

I'm still trying to figure out whether it's a real bug or just we're relying on too many variables.

labath added a comment.Nov 9 2020, 1:22 AM

Interesting. It would still probably be useful to mention the uncertain state of this approach in the code somewhere...

mgorny planned changes to this revision.Nov 9 2020, 1:43 AM

Yeah, I'm not going to proceed until I get more feedback from FreeBSD end.

mgorny updated this revision to Diff 303957.Nov 9 2020, 12:26 PM

Added a verbose comment why we're doing that.

This revision is now accepted and ready to land.Nov 9 2020, 12:26 PM
mgorny updated this revision to Diff 304138.Nov 10 2020, 4:31 AM

clang-format

labath accepted this revision.Nov 10 2020, 5:00 AM
labath added inline comments.
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
214

maybe this cast could be inside GetCurrentThread? Linux has it that way...

mgorny added inline comments.Nov 10 2020, 5:05 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
214

I suppose that makes sense. I'll try it in a followup commit.

Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 5:18 AM
mgorny added inline comments.Nov 10 2020, 5:22 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
214

Actually, we're using this method exactly once, so I don't think it makes sense at this point. However, I'll keep it in mind in case it becomes used more frequently.