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

Xenia Ragiadakou posted 1 patch 1 year, 8 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220704072232.117517-1-burzalodowa@gmail.com
xen/arch/arm/include/asm/processor.h | 1 -
xen/arch/arm/traps.c                 | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)
[PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago
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@gmail.com>
---
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 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.

 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
Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Bertrand Marquis 1 year, 8 months ago
Hi Xenia,

> On 4 Jul 2022, at 08:22, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
> ---
> 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
> 
Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Luca Fancellu 1 year, 8 months ago
>> 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.
> 

In addition to this, if we don’t want to provide a justification for those, since they seems to me
code related to debugging they can be removed from “production” code in some way.

> Cheers
> Bertrand

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago

On 7/4/22 10:58, Luca Fancellu wrote:
> 
>>> 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.
>>
> 
> In addition to this, if we don’t want to provide a justification for those, since they seems to me
> code related to debugging they can be removed from “production” code in some way.

Rule 8.7 is advisory, so I think that formal justification of deviations 
is not necessary.

> 
>> Cheers
>> Bertrand
> 

-- 
Xenia

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Luca Fancellu 1 year, 8 months ago

> On 4 Jul 2022, at 09:06, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> 
> 
> On 7/4/22 10:58, Luca Fancellu wrote:
>>>> 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.
>>> 
>> In addition to this, if we don’t want to provide a justification for those, since they seems to me
>> code related to debugging they can be removed from “production” code in some way.
> 
> Rule 8.7 is advisory, so I think that formal justification of deviations is not necessary.

Yes that is true, in that case we would only need to document it without a formal justification, however
if the codebase doesn’t include them (because not in production code) I guess the problem doesn’t exist.

> 
>>> Cheers
>>> Bertrand
> 
> -- 
> Xenia

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Bertrand Marquis 1 year, 8 months ago

> On 4 Jul 2022, at 09:25, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
> 
> 
> 
>> On 4 Jul 2022, at 09:06, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>> 
>> 
>> 
>> On 7/4/22 10:58, Luca Fancellu wrote:
>>>>> 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.
>>>> 
>>> In addition to this, if we don’t want to provide a justification for those, since they seems to me
>>> code related to debugging they can be removed from “production” code in some way.
>> 
>> Rule 8.7 is advisory, so I think that formal justification of deviations is not necessary.
> 
> Yes that is true, in that case we would only need to document it without a formal justification, however
> if the codebase doesn’t include them (because not in production code) I guess the problem doesn’t exist.

Having the production code using static and the non production using external linkage would be kind of weird here.
I think having them always with external linkage with a justification is the cleanest way.

Cheers
Bertrand

> 
>> 
>>>> Cheers
>>>> Bertrand
>> 
>> --
>> Xenia

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Julien Grall 1 year, 8 months ago
Hi,

On 04/07/2022 09:28, Bertrand Marquis wrote:
>> On 4 Jul 2022, at 09:25, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>
>>
>>
>>> On 4 Jul 2022, at 09:06, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>>
>>>
>>>
>>> On 7/4/22 10:58, Luca Fancellu wrote:
>>>>>> 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.
>>>>>
>>>> In addition to this, if we don’t want to provide a justification for those, since they seems to me
>>>> code related to debugging they can be removed from “production” code in some way.
>>>
>>> Rule 8.7 is advisory, so I think that formal justification of deviations is not necessary.
>>
>> Yes that is true, in that case we would only need to document it without a formal justification, however
>> if the codebase doesn’t include them (because not in production code) I guess the problem doesn’t exist.
> 
> Having the production code using static and the non production using external linkage would be kind of weird here.
> I think having them always with external linkage with a justification is the cleanest way.

+1 this is what I was going to answer :).

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Luca Fancellu 1 year, 8 months ago
>>>>>> 
>>>>>> As said I would vote for external linkage here but would be nice to have other
>>>>>> developers view on this.
>>>>>> 
>>>>> In addition to this, if we don’t want to provide a justification for those, since they seems to me
>>>>> code related to debugging they can be removed from “production” code in some way.
>>>> 
>>>> Rule 8.7 is advisory, so I think that formal justification of deviations is not necessary.
>>> 
>>> Yes that is true, in that case we would only need to document it without a formal justification, however
>>> if the codebase doesn’t include them (because not in production code) I guess the problem doesn’t exist.
>> Having the production code using static and the non production using external linkage would be kind of weird here.
>> I think having them always with external linkage with a justification is the cleanest way.
> 
> +1 this is what I was going to answer :).
> 

Yes probably I didn’t explained very well, I’m in favour for external linkage, hence we are going to have an advisory to
document.

I was just thinking if we need to document that *if* the codebase doesn’t include them, which is a comment not related
to this patch so apologies for the noise on that. 

> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation
Posted by Xenia Ragiadakou 1 year, 8 months ago

On 7/4/22 11:54, Luca Fancellu wrote:
>>>>>>>
>>>>>>> As said I would vote for external linkage here but would be nice to have other
>>>>>>> developers view on this.
>>>>>>>
>>>>>> In addition to this, if we don’t want to provide a justification for those, since they seems to me
>>>>>> code related to debugging they can be removed from “production” code in some way.
>>>>>
>>>>> Rule 8.7 is advisory, so I think that formal justification of deviations is not necessary.
>>>>
>>>> Yes that is true, in that case we would only need to document it without a formal justification, however
>>>> if the codebase doesn’t include them (because not in production code) I guess the problem doesn’t exist.
>>> Having the production code using static and the non production using external linkage would be kind of weird here.
>>> I think having them always with external linkage with a justification is the cleanest way.
>>
>> +1 this is what I was going to answer :).
>>
> 
> Yes probably I didn’t explained very well, I’m in favour for external linkage, hence we are going to have an advisory to
> document.
> 
> I was just thinking if we need to document that *if* the codebase doesn’t include them, which is a comment not related
> to this patch so apologies for the noise on that.
> 
>> Cheers,
>>
>> -- 
>> Julien Grall
> 

I would like to mention that show_execution_state() is also available 
for dumping the state but probably you need them for more fine grained 
debugging.
I will wait until tomorrow in case there is further input on this and I 
will send another patch, if necessary.

-- 
Xenia