arch: introduce arch_stack_walk() and add implementation for RISCV

[ycsin]

While reviewing #72890, I think the stacktrace inplementation in RISCV can probably be factored out so that it can be useful for #72890.

And seems like Linux already has an API for such use case, I guess we can probably just steal it over..

Added an implementation for RISC-V, tested and works on a modified version of #72890

Added a shell command to unwind a stack:

uart:~$ kernel threads
Scheduler: 11 since last call
Threads:
*0x80017138 shell_uart
        options: 0x0, priority: 14 timeout: 0
        state: queued, entry: 0x800029ac
        stack size 3072, unused 1316, usage 1756 / 3072 (57 %)

 0x80017ca8 sysworkq
        options: 0x1, priority: -1 timeout: 0
        state: pending, entry: 0x80006842
        stack size 1024, unused 644, usage 380 / 1024 (37 %)

 0x800177e0 idle
        options: 0x1, priority: 15 timeout: 0
        state: , entry: 0x800065ae
        stack size 512, unused 180, usage 332 / 512 (64 %)

 0x80017950 main
        options: 0x1, priority: 0 timeout: 13
        state: suspended, entry: 0x80006326
        stack size 4096, unused 3604, usage 492 / 4096 (12 %)

uart:~$ kernel unwind 0x80017ca8
Unwinding 0x80017ca8 sysworkq
ra: 0x80007114 [z_swap+0x58]
ra: 0x80007ae8 [z_sched_wait+0x10]
ra: 0x8000689a [work_queue_main+0x58]
ra: 0x800006de [z_thread_entry+0x2e]

[andyross]

Some notes. Overall this looks clean to me. Note that there is also a ton of per-arch stack walking code that e.g. does LOG_ERR() or printk() to hand format stack traces that we should look at replacing with a portable one based on this.

[andyross]

Do we need this as a Kconfig? Seems like a feature that, if unused, would be dropped from the link. The only other reason to make it tunable would be to optimize build time for uncalled code, but again this is tiny and IMHO we don't want to be playing games like that at the arch level.

[ycsin]

Removed CONFIG_ARCH_STACKWALK but I retained the CONFIG_ARCH_HAS_STACKWALK, which is used to guard the CONFIG_ARCH_STACKWALK_MAX_FRAMES. CONFIG_ARCH_STACKWALK_MAX_FRAMES (like the CONFIG_EXCEPTION_STACK_TRACE_MAX_FRAMES) will be helpful to make sure that we dont stuck in an infinite loop, LMK what you think

[ycsin]

Removed the implementation for x86 & arm64 as they probably do not work properly outside of the fatal use case.

[ycsin]

cc @npitre

[ycsin]

arch_ interfaces are not something meant to be implemented out-of-tree

Q: Does this apply to the OOT archs? Do we actually support OOT archs?

[npitre]

Your commit log says:

The fp-based implementation of stack-unwinding when the esf is
NULL came from a5d86b8b077466384f2c2ea6771498a1e265bb95.

But...

$ git log a5d86b8b077466384f2c2ea6771498a1e265bb95
fatal: bad object a5d86b8b077466384f2c2ea6771498a1e265bb95

You may refer to a git hash only if it is merged upstream. Pull requests are
applied with a "rebase" not with a "merge" so any hash from any private tree
will be different once applied.

Then, in the code:

       if ((esf == NULL) && (csf == NULL)) {
               return;
       } else if (esf != NULL) {
               fp = esf->s0;
               ra = esf->mepc;
       } else if ((csf == NULL) || csf == &_current->callee_saved) {
               esf = *((struct arch_esf **)(((uintptr_t)_current_cpu->irq_stack) - 16));
               fp = esf->s0;
               ra = (uintptr_t)walk_stackframe;
       } else {
               fp = csf->s0;
               ra = csf->ra;
       }

This would require some comments. Way too much magic is going on here.
For instance, what is that irq_stack - 16 all about? I probably can guess
only because I'm familiar with the IRQ stack switch as I wrote it. But even
so I'm not completely sure. Now imagine anybody else.

Also, commenting how each of these esfand csf combinations may
come to be would help as well.

Furthermore, if both esf and csf are NULL then the first if is true.
If the second if is false, that means esf is null and therefore csf
must be non null... meaning the csf == NULL condition in the 3rd if will
never be true.

So please don't hesitate to comment such tricky code. Otherwise a year from
now even you won't remember why you wrote it.

[ycsin]

Thanks for the review

You may refer to a git hash only if it is merged upstream. Pull requests are
applied with a "rebase" not with a "merge" so any hash from any private tree
will be different once applied.

I've updated the commit to point at the PR instead

Furthermore, if both esf and csf are NULL then the first if is true.
If the second if is false, that means esf is null and therefore csf
must be non null... meaning the csf == NULL condition in the 3rd if will
never be true.

Good catch, I've reoredered the if conditionals and added some comments to it, could you please take another look?

[ycsin]

Increase some of the timeouts in tests as the stacktrace requires a little bit more time than before, and rebased on main

[ycsin]

I'm not sure if it's just my laptop's performance is bad, the test_inc_concurrency alone takes 100+ seconds to finish

START - test_inc_concurrency
type 0: cnt 60000, spend 1299 ms
type 1: cnt 60000, spend 53075 ms
type 2: cnt 60000, spend 54327 ms
 PASS - test_inc_concurrency in 108.705 seconds

Not sure if 120s is enough in upstream

[ycsin]

ping @andyross @dcpleung, could you please take another look? Thanks