I this line still needed ?

I think this would leave out almost all threads. The issue is that we set up td_frame e.g. on syscall entry and keep it there. Fast interrupts and some MD handlers e.g. local timer when calling into MI code, set up td_intr_frame but do not touch td_frame. Also td_frame could be invalid or NULL for kernel threads that never trap from userspace.

I meant to write td_intr_frame, but even that is not enough since we do not set it for rendezvous interrupts.

One solution would be to re-add IPI_TRACE and handle it from ipi_bitmap_handler(), but then the code would no longer be MI. Another possibility is to modify MD callers of smp_rendezvous_action() to pass a trapframe, and have smp_rendezvous_action() use it to set td_intr_frame. Do you see any problems with the second approach?

I think #2 would work, but I somewhat irrationally do not like it.

That said, why do you need to check for usermode interruption ? It is displaying something like <empty> or <usermode> vs. one- or two-frames stack.

It is not really necessary to check, since we validate that the frame pointer lies within the kernel stack. But, we still need a way to get the interrupted thread's frame pointer. The use of td_pcb below is also wrong for running threads.

The validation of the FP is MD and I do not think that all implementations of stack_capture() check this. So I suspect now that it is preferable to keep the implementation x86-specific and use a new IPI vector.

Well, ok.

Is there anything apart from thread lock being held here which causes the stack_save_td caller to run with interrupts disabled?

I think there are 2 issues here:

1. callers should be required to not have interrutps disabled, perhaps modulo except for thread lock. then there is no need to check spinlock_count in the first place.
2. since thread lock is dropped nothing prevents the target thread from exiting and getting reallocated. I don't see a good way to either prevent it or verify it did not happen. The least which can be done is validating in the dumping handler that the thread belongs to the expected process (by that i mean both the pointer and process start time match -- see microuptime(&p2->p_stats->p_start) in fork).

I just realized the caller is already required to not hold any other spinlocks since smp_rendezvous_cpu must be called with interrupts enabled. Therefore the spinlock_count check can be just removed as it is.

Is this really assertable? can the thread become swapped after the lock is dropped below? looks like this should be just checked

As noted below, the caller prevents this either by holding the proc lock, or by ensuring that the thread is not already running, in which case the thread lock ensures that it cannot transition to TDS_SWAPPED.

I made a lot of mistakes in this diff. You are right that we do not need to check spinlock_count. I meant to check curthread's spinlock count, not td's, but it is not needed anyway. In cases where the thread may be running, we hold the process lock, which prevents both swapping and exiting. I will add an assertion for this.

I think you do not need intr_success, it is enough to check that the stack was indeed captured in stack_save_td().

Wouldn't it better to gather all asserts at the start of the function, so that the consumer not miss anything in casual testing ?

Don't you need to clear intr_pending before ipi ? Also I would suggest to prefix all static vars in this file with e.g. stack_, since they are visible in debugger.

Don't you want to remove stack_save_td_running() and keep stack_save()?