| Message ID | 20220704072232.117517-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: traps: fix MISRA C 2012 Rule 8.7 violation | expand |
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 >
>> 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
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 >
> 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
> 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
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,
>>>>>> >>>>>> 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
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.
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;
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(-)