Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Xenia,

> On 4 Jul 2022, at 08:22, Xenia Ragiadakou <burzalodowa@xxxxxxxxx> wrote:
> 
> The functions show_registers() and show_stack() are referenced only in 
> traps.c.
> Change their linkage from external to internal by adding the storage-class
> specifier static to their definitions and by removing show_registers() from
> asm/processor.h header file.
> 
> Also, this patch resolves a MISRA C 2012 Rule 8.4 violation warning about the
> function show_stack().
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> ---
> I am not 100% sure about this patch.
> I think show_stack() should be declared the same way as show_registers().
> So either both of them will be declared with external linkage or both of them
> will be declared with internal linkage.

I think that those 2 should be declared with external linkage with a comment
explaining why they are. For me those are useful when developing or debugging
and I sometime call those to force dumping the status.
So I would vote to keep the external linkage.

> I decided to declare both of them static because they are referenced only in
> traps.c but I could have, also, add the declaration of show_stack() in
> asm/processor.h header instead. Rule 8.7 is advisory.

As said I would vote for external linkage here but would be nice to have other
developers view on this.

Cheers
Bertrand

> 
> xen/arch/arm/include/asm/processor.h | 1 -
> xen/arch/arm/traps.c                 | 4 ++--
> 2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h 
> b/xen/arch/arm/include/asm/processor.h
> index 4188ec6bfb..75c680ae9a 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -558,7 +558,6 @@ extern register_t __cpu_logical_map[];
> void panic_PAR(uint64_t par);
> 
> void show_execution_state(const struct cpu_user_regs *regs);
> -void show_registers(const struct cpu_user_regs *regs);
> //#define dump_execution_state() 
> run_in_exception_handler(show_execution_state)
> #define dump_execution_state() WARN()
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 785f2121d1..9398ceeed5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -931,7 +931,7 @@ static void _show_registers(const struct cpu_user_regs 
> *regs,
>     printk("\n");
> }
> 
> -void show_registers(const struct cpu_user_regs *regs)
> +static void show_registers(const struct cpu_user_regs *regs)
> {
>     struct reg_ctxt ctxt;
>     ctxt.sctlr_el1 = READ_SYSREG(SCTLR_EL1);
> @@ -1146,7 +1146,7 @@ static void show_trace(const struct cpu_user_regs *regs)
>     printk("\n");
> }
> 
> -void show_stack(const struct cpu_user_regs *regs)
> +static void show_stack(const struct cpu_user_regs *regs)
> {
>     register_t *stack = STACK_BEFORE_EXCEPTION(regs), addr;
>     int i;
> -- 
> 2.34.1
>