Page MenuHomePhabricator

[lldb] [Process/NetBSD] Improve threading support

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



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

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

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


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

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


Maybe GetID() -> pid? Same later.


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.


We already log the issue above.


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


Indeed, good catch.


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.


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.


Maybe this could be an assert ?


(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.


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.


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.Nov 12 2019, 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.Nov 21 2019, 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.Nov 22 2019, 12:13 AM

Looks good. Thanks for doing those tests.


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.Nov 22 2019, 12:13 AM
mgorny marked an inline comment as done.Nov 22 2019, 12:39 AM
mgorny added inline comments.

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.Nov 22 2019, 6:16 AM

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 TranscriptNov 25 2019, 11:13 AM