[PATCH] riscv: add backtrace support

[Ben Dooks]

On 15/05/2023 14:03, Ben Dooks wrote:
> When debugging, it is useful to have a backtrace to find
> out what is in the call stack as the previous function (RA)
> may not have been the culprit.
> 
> Since this adds size to the build, do not add it by default
> and avoid putting it in the SPL build if not needed.

Hi, has anyone had time to review this?

As a note, my sifive.com address may go soon, so please
add ben.dooks at codethink.co.uk to any followups.

> Signed-off-by: Ben Dooks <ben.dooks at sifive.com>
> ---
>   arch/riscv/Kconfig          | 10 ++++++++++
>   arch/riscv/Makefile         |  4 ++++
>   arch/riscv/cpu/start.S      |  1 +
>   arch/riscv/lib/interrupts.c | 35 +++++++++++++++++++++++++++++++++++
>   4 files changed, 50 insertions(+)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index f6ed05906a..3f2316cfb5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -98,6 +98,16 @@ config ARCH_RV64I
>   
>   endchoice
>   
> +config FRAMEPOINTER
> +	bool "Build with frame pointer for stack unwinding"
> +	help
> +	  Choose this option to use the frame pointer so the stack can be
> +	  unwound if needed. This is useful for tracing where faults came
> +	  from as the source may be several functions back
> +
> +	  If you say Y here, then the code size will be increased due to
> +	  having to store the fp.
> +
>   choice
>   	prompt "Code Model"
>   	default CMODEL_MEDLOW
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 4963b5109b..0cb60c7c7e 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -45,6 +45,10 @@ endif
>   ARCH_FLAGS = -march=$(RISCV_MARCH) -mabi=$(ABI) \
>   	     -mcmodel=$(CMODEL)
>   
> +ifeq ($(CONFIG_$(SPL_)FRAMEPOINTER),y)
> +	ARCH_FLAGS += -fno-omit-frame-pointer
> +endif
> +
>   PLATFORM_CPPFLAGS	+= $(ARCH_FLAGS)
>   CFLAGS_EFI		+= $(ARCH_FLAGS)
>   
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index dad22bfea8..3d13722615 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -396,6 +396,7 @@ call_board_init_r:
>    */
>   	mv	a0, s3			/* gd_t */
>   	mv	a1, s4			/* dest_addr */
> +	mv	s0, zero		/* fp == NULL */
>   
>   /*
>    * jump to it ...
> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
> index e966afa7e3..db3d7e294b 100644
> --- a/arch/riscv/lib/interrupts.c
> +++ b/arch/riscv/lib/interrupts.c
> @@ -53,6 +53,40 @@ static void show_regs(struct pt_regs *regs)
>   #endif
>   }
>   
> +#if defined(CONFIG_FRAMEPOINTER) || defined(CONFIG_SPL_FRAMEPOINTER)
> +static void show_backtrace(struct pt_regs *regs)
> +{
> +	uintptr_t *fp = (uintptr_t *)regs->s0;
> +	unsigned count = 0;
> +	ulong ra;
> +
> +	printf("backtrace:\n");
> +
> +	/* there are a few entry points where the s0 register is
> +	 * set to gd, so to avoid changing those, just abort if
> +	 * the value is the same */
> +	while (fp != NULL && fp != (uintptr_t *)gd) {
> +		ra = fp[-1];
> +		printf("backtrace %2d: FP: " REG_FMT " RA: " REG_FMT,
> +		       count, (ulong)fp, ra);
> +
> +		if (gd && gd->flags & GD_FLG_RELOC)
> +			printf(" - RA: " REG_FMT " reloc adjusted\n",
> +			ra - gd->reloc_off);
> +		else
> +			printf("\n");
> +
> +		fp = (uintptr_t *)fp[-2];
> +		count++;
> +	}
> +}
> +#else
> +static void show_backtrace(struct pt_regs *regs)
> +{
> +	printf("No backtrace support enabled\n");
> +}
> +#endif
> +
>   /**
>    * instr_len() - get instruction length
>    *
> @@ -119,6 +153,7 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs)
>   		       epc - gd->reloc_off, regs->ra - gd->reloc_off);
>   
>   	show_regs(regs);
> +	show_backtrace(regs);
>   	show_code(epc);
>   	show_efi_loaded_images(epc);
>   	panic("\n");

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html