dtrace: dtrace_getpcstack() tweaks for riscv

[mhorne]

Summary

Backtraces for fbt probes are missing the caller's frame. Despite what
the comment claims, we do need to insert this manually. In fbt_invop(),
set cpu_dtrace_caller to be the return address, not addr.

We should not increment aframes within this function, since we begin the
main loop by unwinding past the current frame.

Plus some very small comment/style tweaks.

[jhb]

I think this looks fine but am happy to give others a chance to chime in.

sys/cddl/dev/dtrace/riscv/dtrace_isa.c

92

I suspect this was a stale comment copied from arm64? At one point arm64 constructed the start of its trap frames to look like a normal frame so that trap frames didn't need custom unwinding IIRC. (I think RISC-V sort of did this as well)

[mhorne]

For reasons that are totally beyond me, this change exposes two additional frames into the backtrace, not the one I would expect by removing the aframes++ line. I compensate for this in the child revisions.

Example backtrace for fbt::pmap_enter_quick_locked:entry probe before this change:

kernel`vm_map_pmap_enter+0x220
kernel`vm_map_insert+0x460
kernel`vm_map_fixed+0x130
kernel`elf64_map_insert+0x18a
kernel`elf64_load_sections+0x1c0
kernel`exec_elf64_imgact+0xb04
kernel`kern_execve+0x592
kernel`sys_execve+0x52
kernel`do_trap_user+0x23c

And after:

kernel`pmap_enter_object+0xd8
kernel`do_trap_supervisor+0x9e
kernel`cpu_exception_handler_supervisor+0x70
kernel`vm_map_pmap_enter+0x220
kernel`vm_map_insert+0x460
kernel`vm_map_fixed+0x130
kernel`elf64_map_insert+0x18a
kernel`elf64_load_sections+0x1c0
kernel`exec_elf64_imgact+0xb04
kernel`kern_execve+0x592
kernel`sys_execve+0x52
kernel`do_trap_user+0x23c

[markj]

sys/cddl/dev/dtrace/riscv/dtrace_isa.c

102

Would it be more accurate to say that fbt_invop() records the saved return address at the time that an FBT probe fired? Or am I misunderstanding. FBT probes don't have callers, so this comment isn't quite clear to me.

sys/cddl/dev/fbt/riscv/fbt_isa.c

60

Is the subtraction actually required in order to get sane backtraces?

[mhorne]

sys/cddl/dev/dtrace/riscv/dtrace_isa.c

92

Yeah, it does seem that arm and arm64 construct a proper frame in the exception handler, but on riscv we do not. There is some special unwinding logic in db_stack_trace_cmd(), for instance.

I will experiment with constructing a proper trap frame in the riscv exception handler, but so far the existing routine is not super amenable to that.

102

Yeah my terminology is off. The 'caller' here means the caller of the function being traced. I'll adjust it.

sys/cddl/dev/fbt/riscv/fbt_isa.c

60

No, it is for accuracy only. unwind_frame() performs the same adjustment.

[jhb]

sys/cddl/dev/dtrace/riscv/dtrace_isa.c

92

To be clear, I don't think there is any need to change the exception frame layout. Debuggers like kgdb hardcode the layout of struct trapframe so it's probably best to leave the frame layout the way it is.

[mhorne]

Tweak language in comment.

[mhorne]

sys/cddl/dev/dtrace/riscv/dtrace_isa.c

92

Yes, I'm with you there. What I meant was having the exception handler construct a stack frame in addition to the trap frame, leaving the latter untouched.

I tested this idea and although it did work to remove the special case from db_stack_trace_cmd(), it didn't simplify anything wrt dtrace unwinding; in fact it made FBT return probes a special case. So, I do not intend to deviate from the approach in this revision.