Add ptrace(2) reporting for LWP events.

[jhb]

Summary

Add ptrace(2) reporting for LWP events.

Add two new LWPINFO flags: PL_FLAG_BORN and PL_FLAG_EXITED for reporting
thread creation and destruction. Newly created threads will stop to report
PL_FLAG_BORN before returning to userland and exiting threads will stop to
report PL_FLAG_EXIT before exiting completely. Both of these events are
only enabled and reported if PT_LWP_EVENTS is enabled on a process.

Call kern_thr_exit() instead of duplicating it.

This was already missing the racct_subr() call from kern_thr_exit(), but
this will now also add TDB_EXIT ptrace events for threads that exit this
way.

Test Plan

• Run the test case.
• Use PT_LWP_EVENTS with a new WIP thread target for gdb7.

[jhb]

Aside from debuggers I will also probably make use of this in truss at some point to properly handle system calls like 'thr_exit()' where there is no corresponding system call exit event (instead I would use the LWP exit to simulate the system call exit).

sys/kern/kern_thr.c

328

I had originally placed the TDB_EXIT stop here, but that resulted in sleeping with the tidhash_lock held. That is when I resorted to the p_exitingthreads scheme. The general idea is that threads pre-reserve a "slot" for not being the last thread to exit, but this means the last thread standing has to wait for those slots to drain if it is the last thread.

sys/kern/kern_thread.c

953–954

I intend to commit this first as a separate commit but included it here for context. (The second half of the commit message above is actually for this change.) We are already missing an 'racct' call here and I would have to duplicate the ptrace stop, etc.

One caveat is that kern_thr_exit() is now seemingly misnamed and/or in the wrong place as it probably should be a function in this file as it isn't really specific to the "thr" system calls.

[kib]

I.e. the API is not symmetric, it does not generate the event for the last thread. Debugger must destroy its state for the last thread when wait(2) reports exit. This is somewhat strange, but fine.

I convinced myself that p_exitingthreads mechanism is correct. You could add more comments why it was implemented, at least moving the inline text from the review into the code comment.

Dropping process lock while the tid is removed from the hash list appeared to be innocent as well.

[jhb]

In D4703#99384, @kib wrote:

I.e. the API is not symmetric, it does not generate the event for the last thread. Debugger must destroy its state for the last thread when wait(2) reports exit. This is somewhat strange, but fine.

OTOH, we don't report thread birth for the first thread, so in that way it is symmetric. Initial thread birth is handled via attach or a new fork child event, and final thread exit is reported via process exit. In both gdb (where I've used this) and truss (where I've modeled it in my head) having a thread exit event for the last thread would actually add more work (it would have to be checked for and ignored while waiting for the exit event).

I convinced myself that p_exitingthreads mechanism is correct. You could add more comments why it was implemented, at least moving the inline text from the review into the code comment.

Ok. I have wondered if it would be ok to remove the sleep from the "last" thread since once it called exit(0) it would then wait for the pending exit threads to all be resumed by the debugger when reverting to a single thread. The only question is if it would force an early exit of some other thread that was trying to call pthread_exit(). I think it wouldn't though since all the other threads would have had to already bumped p_exitingthread and either be stopped in ptracestop() or be in tidhash_remove(). In that case the thread isn't in userland so the single threading request in exit() should just let those exiting threads run to completion to finish their exit. If so, I believe I can remove the sleep/wakeup for &p->p_exitingthreads. I guess the only question is if the exit is pending when the thread resumes in ptracestop() will
it end up recursing into kern_thr_exit() via thread_suspend_check() it will it always return to the caller to finish.

It looks like ptracestop() just calls thread_suspend_switch() directly and never calls thread_suspend_check(), so I think it will be fine. Oh, but it does break out of the loop without blocking if P_SINGLE_EXIT is called. This could result in thread exit events not being reported. (Suppose the threads are preempted while in the kernel but before reporting the ptrace stop, the userland pthread_join in the main thread can complete since the thread library has finished its exit and the main thread might then reasonably call exit() believing all threads to be gone. This race happens regardless of the sleep on &p->p_exitingthreads and is the other reason that I think I can remove that sleep: any race that exists also exists if the main thread calls exit() directly so it can't be handled in kern_thr_exit()). My initial hacky idea is to not break out of the loop early for P_SINGLE_EXIT if TDB_EXIT is set.

Dropping process lock while the tid is removed from the hash list appeared to be innocent as well.

The code for suspended threads already did this as well. I think the overlapping locks were just to make the 'p->p_numthreads == 1' check atomic and the p_exitingthreads is what replaces the overlapping locks.

[kib]

In D4703#99634, @jhb wrote:

It looks like ptracestop() just calls thread_suspend_switch() directly and never calls thread_suspend_check(), so I think it will be fine. Oh, but it does break out of the loop without blocking if P_SINGLE_EXIT is called. This could result in thread exit events not being reported. (Suppose the threads are preempted while in the kernel but before reporting the ptrace stop, the userland pthread_join in the main thread can complete since the thread library has finished its exit and the main thread might then reasonably call exit() believing all threads to be gone. This race happens regardless of the sleep on &p->p_exitingthreads and is the other reason that I think I can remove that sleep: any race that exists also exists if the main thread calls exit() directly so it can't be handled in kern_thr_exit()). My initial hacky idea is to not break out of the loop early for P_SINGLE_EXIT if TDB_EXIT is set.

This could work, at least I do not see why not.

BTW, we already have p_exitthreads counter which ticks for threads coming through sched_throw(). Might be, if keeping the p_exitingthread at all, rename it.

[kib]

I think that you should add execve(2) to the test program. In fact, the precise handling of the events for exiting threads is not quite needed when the process exits, since debugger can deterministically deduce that all threads are dead when the process is dead (and you do not report the last thread death anyway). But for exec, the process and the execing thread are survivors, while other threads are not.

[jhb]

• Add a comment about p_exitingthreads.
• Rename p_exitingthreads to p_pendingexits.
• No need to sleep in kern_thr_exit() on p_pendingexits.
• Add a test for LWP events when a child thread calls execve().
• PL_FLAG_SCE is not set for the thread terminated by single threading.

[jhb]

In D4703#99764, @kib wrote:

I think that you should add execve(2) to the test program. In fact, the precise handling of the events for exiting threads is not quite needed when the process exits, since debugger can deterministically deduce that all threads are dead when the process is dead (and you do not report the last thread death anyway). But for exec, the process and the execing thread are survivors, while other threads are not.

Done. In fact I had an old kernel around where thread_suspend_switch() was not calling kern_thr_exit() yet but was still doing the inline version that didn't do a ptrace stop and the test failed on it but works fine on the latest version with kern_thr_exit() in thread_suspend_check().

I also renamed the member to 'p_pendingexits' and removed the explicitly sleep in kern_thr_exit().

Arguably sys_thr_exit() should not return to libc but should call exit1() directly as the other ABIs do (or kern_thr_exit() should call it directly and allow the caller to pass in an exit value to use in that case).

[jhb]

sys/kern/kern_sig.c

2539

So this was more of a question in that should we always clear TDB_XSIG here? In particular, syscall traps do not check P_TRACED under the PROC_LOCK, so it might be cleared in between when it was checked and when the thread acquires the lock before entering ptracestop(). In that case the loop would never be entered and this function would return with TDB_XSIG still set. I will go ahead and commit the LWP events without this comment, but I think a followup commit to always clear TDB_XSIG here might be warranted? (Either that or check for P_TRACED again under the PROC_LOCK in the system call traps?)

[jhb]

Committed in rS292894

[kib]

sys/kern/kern_sig.c

2539

Leaked TDB_XSIG indeed might cause spurious report of PL_EVENT_SIGNAL, so I agree that clearing it would fix this. But, note the if (!P_TRACED) break; and if (P_SINGLE_EXIT) break; cases after stopme. I am not sure if it could be useful someday to see that thread was terminated while debug event was pending.

Might be, do not set TDB_XSIG and return early from ptracestop() if P_TRACED is not set ? May be not, and just clear it at the end for now. I think both variants are fine.