Check for invalid pc in unwind_frame

[mhorne]

Summary

Rather than expecting callers to validate the PC of the returned frame,
we are better off just returning an error. This helps
db_trace_stack_cmd, which would unwind one frame too far for proc0
because it did not do this.

Test Plan

Before:

db> bt 0
Tracing pid 0 tid 100000 td 0xffffffc000726b00
sched_switch() at sched_switch+0x572
mi_switch() at mi_switch+0x186
sleepq_wait() at sleepq_wait+0x170
sleepq_timedwait() at sleepq_timedwait+0x3c
_sleep() at _sleep+0x23c
swapper() at swapper+0xac
mi_startup() at mi_startup+0x296
_start() at _start+0x18c
(null)() at -0x4

After:

db> bt 0
Tracing pid 0 tid 100000 td 0xffffffc000726b00
sched_switch() at sched_switch+0x572
mi_switch() at mi_switch+0x186
sleepq_wait() at sleepq_wait+0x170
sleepq_timedwait() at sleepq_timedwait+0x3c
_sleep() at _sleep+0x23c
swapper() at swapper+0xac
mi_startup() at mi_startup+0x296
_start() at _start+0x18c

[jrtc27]

sys/riscv/riscv/stack_machdep.c

60

The error code isn't checked, so now this will keep unwinding. An existing bug as frame->fp could have been invalid (though presumably the above ad-hoc if is meant to check that up-front?), but made more apparent by this change.

[mhorne]

Check the error from unwind_frame.

[jhb]

I guess this is ok. Nominally if you could have a user pc for things like faults and exceptions, but those all require a custom unwinder in DDB anyway. unwind_frame() doesn't handle them. If we fixed unwind_frame() to learn about trap frames and unwind across them, then you would perhaps not want this check in unwind_frame(). Is this still needed after the changes in D26016?

[mhorne]

In D26015#577980, @jhb wrote:

I guess this is ok. Nominally if you could have a user pc for things like faults and exceptions, but those all require a custom unwinder in DDB anyway. unwind_frame() doesn't handle them. If we fixed unwind_frame() to learn about trap frames and unwind across them, then you would perhaps not want this check in unwind_frame(). Is this still needed after the changes in D26016?

I see what you mean, just because it's not a kernel PC doesn't mean it's invalid, just unsupported in our case. The other change does properly address the issue I was seeing, so let's let this be.