Page MenuHomePhabricator

[lldb] [Process/NetBSD] Improve threading support
ClosedPublic

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

Details

Summary

Implement major improvements to multithreaded program support. Notably,
support tracking new and exited threads, associate signals and events
with correct threads and support controlling individual threads when
resuming.

Firstly, use PT_SET_EVENT_MASK to enable reporting of created and exited
threads via SIGTRAP. Handle TRAP_LWP events to keep track
of the currently running threads.

Secondly, update the signal (both generic and SIGTRAP) handling code
to account for per-thread signals correctly. Signals delivered
to the whole process are reported on all threads, while per-thread
signals and events are reported only to the specific thread.
The remaining threads are marked as 'stopped with no reason'. Note that
NetBSD always stops all threads on debugger events.

Thirdly, implement the ability to set every thread as running, stopped
or single-stepping separately while continuing the process. This also
provides the ability to send a signal to the whole process or to one
of its thread while resuming.

Note 1: this is not perfect but the whole series is good enough to commit and work on fixing remaining bugs afterwards.

Note 2: the series in general fixes a lot of tests but I'm going to wait with test status updates until buildbot confirms.

Diff Detail

Event Timeline

krytarowski added inline comments.Nov 8 2019, 1:16 PM
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
241–243

This shall be always true unless there is a kernel issue.
But this check is fine, I would just add a comment.

248–249

Same here. Isn't there a fixup for PC?

It's not needed in x86 and can be delayed into future.

268

Maybe GetID() -> pid? Same later.

772

I would add an assert thread_id > 0.

mgorny marked 7 inline comments as done.Nov 9 2019, 2:54 AM

I've made the changes locally, I'll test and reupload when I finish updating the whole batch.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
241–243

We already log the issue above.

248–249

I dunno and I don't want to proactively add something I can't test right now.

268

Indeed, good catch.

311–312

Actually, I've decided to remove this block for now rather than adding dummy APIs to make it not crash. We will have to update it to match watchpoint logic later on anyway, so we may as well copy and modify the watchpoint logic instead.

772

Added here and to AddThread() as well.

mgorny updated this revision to Diff 228567.Nov 9 2019, 4:00 AM
mgorny marked 2 inline comments as done.

Update wrt comments. Also remove non-working hardware breakpoint boilerplate as noted.

The code seems reasonable, but I think it would be good include the set of tests which this patch is supposed to fix into the patch itself. (And same for other patches too.) There's also some functionality here that's probably not tested by any of our existing tests, so it would be good to have separate netbsd-specific tests too -- I'm mainly thinking of the code which mangles the resume actions into forms suitable for netbsd -- perhaps a test where one attempts to send two different signals to two threads (and gets an error in return), or when two (but not all) threads get the same signal.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
454–455

Maybe this could be an assert ?

477–481

(just curious) so you need to pass the signal both in the siginfo struct, and also to the PT_CONTINUE operation?

mgorny marked 2 inline comments as done.Nov 11 2019, 1:19 AM

Well, are there any LLDB commands that can actually cause such an event to happen? I haven't been able to find one to send a signal to thread in the first place.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
454–455

Well, looking at the gdb protocol, it probably can't happen indeed. However, when I was writing that I assumed 'ResumeActionList is the limit', and didn't want to assert-crash if things were extended in the future.

477–481

Yes. The former is not strictly necessary but the latter is obligatory.

Well, are there any LLDB commands that can actually cause such an event to happen? I haven't been able to find one to send a signal to thread in the first place.

You can make a gdb-remote test which sends the appropriate packets directly.

mgorny updated this revision to Diff 228907.Tue, Nov 12, 9:40 AM

Included updated on test suite status. I've only un-XFAIL-ed tests that are definitely (well, most likely) fixed. I didn't change the status of tests that are flaky (e.g. concurrent tests that pass only because accidentally concurrent enough). I'll look into adding more tests later.

mgorny updated this revision to Diff 230466.Thu, Nov 21, 8:53 AM

@labath, finally implemented the new tests. Covered two good (signal to one thread and to all threads) and two bad cases (signal to 1<i<n threads, two different signals to two threads). Please review.

labath accepted this revision.Fri, Nov 22, 12:13 AM

Looks good. Thanks for doing those tests.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_vContThreads.py
69–73

It might be nice to actually verify how many signals did the process receive here. I guess you could do that by just checking the output of the signal handler.

This revision is now accepted and ready to land.Fri, Nov 22, 12:13 AM
mgorny marked an inline comment as done.Fri, Nov 22, 12:39 AM
mgorny added inline comments.
lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_vContThreads.py
69–73

I've tried but it claimed to timeout before receiving any output. I suspect the interrupt terminates sleep() in thread, and they exit before the signal handler manages to get run. Or maybe I was doing something wrong.

krytarowski added inline comments.Fri, Nov 22, 6:16 AM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_vContThreads.py
69–73

There is a bug in the kernel with sleep() that it does not return EINTR when interrupted. It shall be fixes as it is imho fatal.

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