Page MenuHomePhabricator

[lldb] [Process/NetBSD] Copy watchpoints to newly-created threads
ClosedPublic

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

Details

Summary

NetBSD ptrace interface does not populate watchpoints to newly-created
threads. Solve this via copying the watchpoints from the current thread
when new thread is reported via TRAP_LWP.

Add a test that verifies that when the user does not have permissions
to set watchpoints on NetBSD, the 'watchpoint set' errors out gracefully
and thread monitoring does not crash on being unable to copy watchpoints
to new threads.

Diff Detail

Event Timeline

How does it deal with security.models.extensions.user_set_dbregs? If there is a handled error than it's fine.

mgorny added a comment.Nov 9 2019, 2:58 AM

How does it deal with security.models.extensions.user_set_dbregs? If there is a handled error than it's fine.

I need to test it. Might turn out it's so broken it's better to address it globally in a separate patch.

mgorny updated this revision to Diff 228954.Tue, Nov 12, 1:39 PM
mgorny marked an inline comment as done.
mgorny edited the summary of this revision. (Show Details)

Included fixed tests + new test from D70050.

mgorny planned changes to this revision.Tue, Nov 12, 1:39 PM
mgorny marked an inline comment as done.
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
280

Note to self: need to add error handling here.

lldb/test/Shell/Watchpoint/netbsd-nouserdbregs.test
6

Note to self: try %clang_host as suggested in D70050.

mgorny updated this revision to Diff 229279.Thu, Nov 14, 4:51 AM

Made dbregs copied only if any watchpoints are enabled, and added error handling.

labath accepted this revision.Fri, Nov 22, 12:18 AM
labath added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h
74–76

Are you sure this is the proper formatting here? It seems fairly odd..

lldb/source/Plugins/Process/NetBSD/NativeThreadNetBSD.cpp
291–292

I'd probably make a covariant override of GetRegisterContext which returns a NativeRegisterContextNetBSD&.

lldb/test/Shell/Watchpoint/netbsd-nouserdbregs.test
6

So, did you try it?

This revision is now accepted and ready to land.Fri, Nov 22, 12:18 AM
mgorny updated this revision to Diff 230604.Fri, Nov 22, 12:56 AM
mgorny marked 2 inline comments as done.

Switched to %clang_host.

Fun fact: things work better if you remove the file before 'readding' it via patch (i.e. ending up appending new contents to old).

mgorny updated this revision to Diff 230609.Fri, Nov 22, 1:34 AM
mgorny marked 3 inline comments as done.

Switched to RegisterContextNetBSD & return type as suggested, and fixed formatting. Had to do some include shifting too.

mgorny added inline comments.Sat, Nov 23, 2:30 AM
lldb/source/Plugins/Process/NetBSD/NativeThreadNetBSD.cpp
291–292

Thanks. I didn't know this was actually legal in C++.

lldb/test/Shell/Watchpoint/netbsd-nouserdbregs.test
6

Sorry, I've forgotten about it. Done now. I presume I don't need native in REQUIRES now.

Will address remaining comments soonish.

labath marked an inline comment as done.Mon, Nov 25, 1:27 AM
labath added inline comments.
lldb/test/Shell/Watchpoint/netbsd-nouserdbregs.test
6

system-netbsd should be enough, but we never really bothered to add "native" even before we had %clang_host. I'm pretty sure a _lot_ of things would blow up if you configured your build so that the default clang target is not the host system.

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