| Message ID | 204bf3ffcdda04d6d6cf072c42b78720e1e85b4d.1690985045.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address MISRA C:2012 Rule 2.1 | expand |
On 02/08/2023 3:38 pm, Nicola Vetrini wrote: > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 5f66c2ae33..015f7b14ab 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d) > { > int ret; > struct vcpu *v; > + enum { > + PROG_iommu_pagetables = 1, > + PROG_shared, > + PROG_paging, > + PROG_vcpu_pagetables, > + PROG_xen, > + PROG_l4, > + PROG_l3, > + PROG_l2, > + PROG_done, > + }; > > BUG_ON(!cpumask_empty(d->dirty_cpumask)); > > @@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d) > #define PROGRESS(x) \ > d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x > > - enum { > - PROG_iommu_pagetables = 1, > - PROG_shared, > - PROG_paging, > - PROG_vcpu_pagetables, > - PROG_xen, > - PROG_l4, > - PROG_l3, > - PROG_l2, > - PROG_done, > - }; > - > case 0: > ret = pci_release_devices(d); > if ( ret ) Why does this get moved? There's no code (reachable or unreachable) in there. This is very subtle logic to start with, and you're moving one part of it away from the comment explaining how the magic works. ~Andrew
On Wed, 2 Aug 2023, Nicola Vetrini wrote: > Variable declarations between a switch statement guard and before > any case label are unreachable code, and hence violate Rule 2.1: > "A project shall not contain unreachable code". > > Therefore the declarations are moved in the smallest enclosing > scope, near other variable definitions. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/arch/x86/cpuid.c | 3 +-- > xen/arch/x86/domain.c | 23 +++++++++++------------ > xen/arch/x86/irq.c | 3 +-- > xen/arch/x86/msr.c | 3 +-- > 4 files changed, 14 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c > index 455a09b2dd..90e1370e90 100644 > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > { > const struct domain *d = v->domain; > const struct cpu_policy *p = d->arch.cpu_policy; > + const struct cpu_user_regs *regs; > > *res = EMPTY_LEAF; > > @@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > */ > switch ( leaf ) > { > - const struct cpu_user_regs *regs; > - > case 0x1: > /* TODO: Rework topology logic. */ > res->b &= 0x00ffffffu; > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 5f66c2ae33..015f7b14ab 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d) > { > int ret; > struct vcpu *v; > + enum { > + PROG_iommu_pagetables = 1, > + PROG_shared, > + PROG_paging, > + PROG_vcpu_pagetables, > + PROG_xen, > + PROG_l4, > + PROG_l3, > + PROG_l2, > + PROG_done, > + }; > > BUG_ON(!cpumask_empty(d->dirty_cpumask)); > > @@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d) > #define PROGRESS(x) \ > d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x > > - enum { > - PROG_iommu_pagetables = 1, > - PROG_shared, > - PROG_paging, > - PROG_vcpu_pagetables, > - PROG_xen, > - PROG_l4, > - PROG_l3, > - PROG_l2, > - PROG_done, > - }; > - > case 0: > ret = pci_release_devices(d); > if ( ret ) > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c > index 6abfd81621..4fd0cc163d 100644 > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1135,6 +1135,7 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) > struct irq_desc *desc = data; > unsigned int i, irq = desc - irq_desc; > irq_guest_action_t *action; > + cpumask_t *cpu_eoi_map; > > spin_lock_irq(&desc->lock); > > @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) > > switch ( action->ack_type ) > { > - cpumask_t *cpu_eoi_map; It is only used by case ACKTYPE_EOI so it can be moved there (with a new block): case ACKTYPE_EOI: { cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); spin_unlock_irq(&desc->lock); on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); return; } } > case ACKTYPE_UNMASK: > if ( desc->handler->end ) > desc->handler->end(desc, 0); > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c > index ecf126566d..0e61e0fe4e 100644 > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -335,11 +335,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) > const struct cpu_policy *cp = d->arch.cpu_policy; > struct vcpu_msrs *msrs = v->arch.msrs; > int ret = X86EMUL_OKAY; > + uint64_t rsvd; > > switch ( msr ) > { > - uint64_t rsvd; > - It is only used by case MSR_INTEL_MISC_FEATURES_ENABLES so it can be moved there > /* Read-only */ > case MSR_IA32_PLATFORM_ID: > case MSR_CORE_CAPABILITIES: > -- > 2.34.1 >
On 03.08.2023 04:13, Stefano Stabellini wrote: > On Wed, 2 Aug 2023, Nicola Vetrini wrote: >> @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >> >> switch ( action->ack_type ) >> { >> - cpumask_t *cpu_eoi_map; > > It is only used by case ACKTYPE_EOI so it can be moved there (with a new > block): > > > case ACKTYPE_EOI: > { > cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); > cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); > spin_unlock_irq(&desc->lock); > on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); > return; > } > } This pattern (two closing braces at the same level) is why switch scope variable declarations were introduced (at least as far as introductions by me go). If switch scope variables aren't okay (which I continue to consider questionable), then this stylistic aspect needs sorting first (if everyone else thinks the above style is okay - with the missing blank line inserted -, then so be it). Jan
On 02.08.2023 16:38, Nicola Vetrini wrote: > Variable declarations between a switch statement guard and before > any case label are unreachable code, and hence violate Rule 2.1: > "A project shall not contain unreachable code". > > Therefore the declarations are moved in the smallest enclosing > scope, near other variable definitions. This needs further clarification: There's no "code" involved here in the sense that a compiler might eliminate it. Yet that's what the rule's rationale is focusing on. There's no even mention of declarations like the ones you move. In particular (and on top of my comment on patch 1) ... > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > { > const struct domain *d = v->domain; > const struct cpu_policy *p = d->arch.cpu_policy; > + const struct cpu_user_regs *regs; > > *res = EMPTY_LEAF; > > @@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > */ > switch ( leaf ) > { > - const struct cpu_user_regs *regs; > - > case 0x1: > /* TODO: Rework topology logic. */ > res->b &= 0x00ffffffu; ... I'm not happy to see scopes of variables widened. Jan
On 03/08/2023 11:01, Jan Beulich wrote: > On 03.08.2023 04:13, Stefano Stabellini wrote: >> On Wed, 2 Aug 2023, Nicola Vetrini wrote: >>> @@ -1169,8 +1170,6 @@ static void cf_check >>> irq_guest_eoi_timer_fn(void *data) >>> >>> switch ( action->ack_type ) >>> { >>> - cpumask_t *cpu_eoi_map; >> >> It is only used by case ACKTYPE_EOI so it can be moved there (with a >> new >> block): >> >> >> case ACKTYPE_EOI: >> { >> cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); >> cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); >> spin_unlock_irq(&desc->lock); >> on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); >> return; >> } >> } > > This pattern (two closing braces at the same level) is why switch scope > variable declarations were introduced (at least as far as introductions > by me go). If switch scope variables aren't okay (which I continue to > consider questionable), then this stylistic aspect needs sorting first > (if everyone else thinks the above style is okay - with the missing > blank line inserted -, then so be it). > > Jan Actually, they can be deviated because they don't result in wrong code being generated. This, modulo the review comments received, is what most of the code would look like if they weren't, with the biggest pain point being that in many cases the choice is either the pattern with blocks for certain clauses or moving them in the enclosing scope, which may be several hundred lines above. If there's agreement on deviating them, I can drop the patches dealing with switches and do a v2 with just the other modifications.
On Thu, 3 Aug 2023, Nicola Vetrini wrote: > On 03/08/2023 11:01, Jan Beulich wrote: > > On 03.08.2023 04:13, Stefano Stabellini wrote: > > > On Wed, 2 Aug 2023, Nicola Vetrini wrote: > > > > @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void > > > > *data) > > > > > > > > switch ( action->ack_type ) > > > > { > > > > - cpumask_t *cpu_eoi_map; > > > > > > It is only used by case ACKTYPE_EOI so it can be moved there (with a new > > > block): > > > > > > > > > case ACKTYPE_EOI: > > > { > > > cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); > > > cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); > > > spin_unlock_irq(&desc->lock); > > > on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); > > > return; > > > } > > > } > > > > This pattern (two closing braces at the same level) is why switch scope > > variable declarations were introduced (at least as far as introductions > > by me go). If switch scope variables aren't okay (which I continue to > > consider questionable), then this stylistic aspect needs sorting first > > (if everyone else thinks the above style is okay - with the missing > > blank line inserted -, then so be it). > > > > Jan > > Actually, they can be deviated because they don't result in wrong code being > generated. > This, modulo the review comments received, is what most of the code would look > like if > they weren't, with the biggest pain point being that in many cases the choice > is either > the pattern with blocks for certain clauses or moving them in the enclosing > scope, which may > be several hundred lines above. If there's agreement on deviating them, I can > drop the patches > dealing with switches and do a v2 with just the other modifications. I think we should deviate them. Good idea to remove them in v2.
On Thu, 3 Aug 2023, Stefano Stabellini wrote: > > Actually, they can be deviated because they don't result in wrong code being > > generated. > > This, modulo the review comments received, is what most of the code would look > > like if > > they weren't, with the biggest pain point being that in many cases the choice > > is either > > the pattern with blocks for certain clauses or moving them in the enclosing > > scope, which may > > be several hundred lines above. If there's agreement on deviating them, I can > > drop the patches > > dealing with switches and do a v2 with just the other modifications. > > I think we should deviate them. Good idea to remove them in v2. We should add a note about this to docs/misra/rules.rst as well? diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index 8f0e4d3f25..e713b0ea99 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -106,7 +106,8 @@ maintainers if you want to suggest a change. * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ - Required - A project shall not contain unreachable code - - + - It is acceptable to declare local variables under a switch + statement block * - `Rule 2.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_06.c>`_ - Advisory
On 03.08.2023 16:22, Nicola Vetrini wrote: > On 03/08/2023 11:01, Jan Beulich wrote: >> On 03.08.2023 04:13, Stefano Stabellini wrote: >>> On Wed, 2 Aug 2023, Nicola Vetrini wrote: >>>> @@ -1169,8 +1170,6 @@ static void cf_check >>>> irq_guest_eoi_timer_fn(void *data) >>>> >>>> switch ( action->ack_type ) >>>> { >>>> - cpumask_t *cpu_eoi_map; >>> >>> It is only used by case ACKTYPE_EOI so it can be moved there (with a >>> new >>> block): >>> >>> >>> case ACKTYPE_EOI: >>> { >>> cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); >>> cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); >>> spin_unlock_irq(&desc->lock); >>> on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); >>> return; >>> } >>> } >> >> This pattern (two closing braces at the same level) is why switch scope >> variable declarations were introduced (at least as far as introductions >> by me go). If switch scope variables aren't okay (which I continue to >> consider questionable), then this stylistic aspect needs sorting first >> (if everyone else thinks the above style is okay - with the missing >> blank line inserted -, then so be it). > > Actually, they can be deviated because they don't result in wrong code > being generated. Only later I recalled Andrew's intention to possibly make use of -ftrivial-auto-var-init. While, unlike I think he said, such declared variables aren't getting in the way of this (neither gcc nor clang warn about them), they also don't benefit from it (i.e. they'll be left uninitialized despite the command line option saying otherwise). IOW I think further consideration is going to be needed here. Jan
>>> >>> This pattern (two closing braces at the same level) is why switch >>> scope >>> variable declarations were introduced (at least as far as >>> introductions >>> by me go). If switch scope variables aren't okay (which I continue to >>> consider questionable), then this stylistic aspect needs sorting >>> first >>> (if everyone else thinks the above style is okay - with the missing >>> blank line inserted -, then so be it). >> >> Actually, they can be deviated because they don't result in wrong code >> being generated. > > Only later I recalled Andrew's intention to possibly make use of > -ftrivial-auto-var-init. While, unlike I think he said, such declared > variables aren't getting in the way of this (neither gcc nor clang > warn about them), they also don't benefit from it (i.e. they'll be > left uninitialized despite the command line option saying otherwise). > IOW I think further consideration is going to be needed here. > > Jan Well, in that case I'm happy if I can contribute in some way to the discussion, but perhaps this is best sorted out in a separate discussion, so that later I can send a v2 reflecting the decision(s).
On Fri, 4 Aug 2023, Jan Beulich wrote: > On 03.08.2023 16:22, Nicola Vetrini wrote: > > On 03/08/2023 11:01, Jan Beulich wrote: > >> On 03.08.2023 04:13, Stefano Stabellini wrote: > >>> On Wed, 2 Aug 2023, Nicola Vetrini wrote: > >>>> @@ -1169,8 +1170,6 @@ static void cf_check > >>>> irq_guest_eoi_timer_fn(void *data) > >>>> > >>>> switch ( action->ack_type ) > >>>> { > >>>> - cpumask_t *cpu_eoi_map; > >>> > >>> It is only used by case ACKTYPE_EOI so it can be moved there (with a > >>> new > >>> block): > >>> > >>> > >>> case ACKTYPE_EOI: > >>> { > >>> cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); > >>> cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); > >>> spin_unlock_irq(&desc->lock); > >>> on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); > >>> return; > >>> } > >>> } > >> > >> This pattern (two closing braces at the same level) is why switch scope > >> variable declarations were introduced (at least as far as introductions > >> by me go). If switch scope variables aren't okay (which I continue to > >> consider questionable), then this stylistic aspect needs sorting first > >> (if everyone else thinks the above style is okay - with the missing > >> blank line inserted -, then so be it). > > > > Actually, they can be deviated because they don't result in wrong code > > being generated. > > Only later I recalled Andrew's intention to possibly make use of > -ftrivial-auto-var-init. While, unlike I think he said, such declared > variables aren't getting in the way of this (neither gcc nor clang > warn about them), they also don't benefit from it (i.e. they'll be > left uninitialized despite the command line option saying otherwise). > IOW I think further consideration is going to be needed here. Let me get this right. Are you saying that if we enable -ftrivial-auto-var-init, due to a compiler limitation, variables declared as follow: switch(var) { int a; char b; case ... do not benefit from -ftrivial-auto-var-init ? So if we moved the variable declarations elsewhere, in one of the two following ways: 1) int a; int b; switch(var) { 2) switch(var) { case 1: { int a; int b; Then -ftrivial-auto-var-init would take effect? If so, I think it is worth a discussion on what to do there. But either way right now I think it is fine to take the switch-related patches out of this series.
On 04.08.2023 22:26, Stefano Stabellini wrote: > On Fri, 4 Aug 2023, Jan Beulich wrote: >> On 03.08.2023 16:22, Nicola Vetrini wrote: >>> On 03/08/2023 11:01, Jan Beulich wrote: >>>> On 03.08.2023 04:13, Stefano Stabellini wrote: >>>>> On Wed, 2 Aug 2023, Nicola Vetrini wrote: >>>>>> @@ -1169,8 +1170,6 @@ static void cf_check >>>>>> irq_guest_eoi_timer_fn(void *data) >>>>>> >>>>>> switch ( action->ack_type ) >>>>>> { >>>>>> - cpumask_t *cpu_eoi_map; >>>>> >>>>> It is only used by case ACKTYPE_EOI so it can be moved there (with a >>>>> new >>>>> block): >>>>> >>>>> >>>>> case ACKTYPE_EOI: >>>>> { >>>>> cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask); >>>>> cpumask_copy(cpu_eoi_map, action->cpu_eoi_map); >>>>> spin_unlock_irq(&desc->lock); >>>>> on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0); >>>>> return; >>>>> } >>>>> } >>>> >>>> This pattern (two closing braces at the same level) is why switch scope >>>> variable declarations were introduced (at least as far as introductions >>>> by me go). If switch scope variables aren't okay (which I continue to >>>> consider questionable), then this stylistic aspect needs sorting first >>>> (if everyone else thinks the above style is okay - with the missing >>>> blank line inserted -, then so be it). >>> >>> Actually, they can be deviated because they don't result in wrong code >>> being generated. >> >> Only later I recalled Andrew's intention to possibly make use of >> -ftrivial-auto-var-init. While, unlike I think he said, such declared >> variables aren't getting in the way of this (neither gcc nor clang >> warn about them), they also don't benefit from it (i.e. they'll be >> left uninitialized despite the command line option saying otherwise). >> IOW I think further consideration is going to be needed here. > > Let me get this right. Are you saying that if we enable > -ftrivial-auto-var-init, due to a compiler limitation, variables > declared as follow: > > switch(var) { > int a; > char b; > > case ... > > do not benefit from -ftrivial-auto-var-init ? Yes, that's my observation with both compilers. Jan
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 455a09b2dd..90e1370e90 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, { const struct domain *d = v->domain; const struct cpu_policy *p = d->arch.cpu_policy; + const struct cpu_user_regs *regs; *res = EMPTY_LEAF; @@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, */ switch ( leaf ) { - const struct cpu_user_regs *regs; - case 0x1: /* TODO: Rework topology logic. */ res->b &= 0x00ffffffu; diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 5f66c2ae33..015f7b14ab 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d) { int ret; struct vcpu *v; + enum { + PROG_iommu_pagetables = 1, + PROG_shared, + PROG_paging, + PROG_vcpu_pagetables, + PROG_xen, + PROG_l4, + PROG_l3, + PROG_l2, + PROG_done, + }; BUG_ON(!cpumask_empty(d->dirty_cpumask)); @@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d) #define PROGRESS(x) \ d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x - enum { - PROG_iommu_pagetables = 1, - PROG_shared, - PROG_paging, - PROG_vcpu_pagetables, - PROG_xen, - PROG_l4, - PROG_l3, - PROG_l2, - PROG_done, - }; - case 0: ret = pci_release_devices(d); if ( ret ) diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 6abfd81621..4fd0cc163d 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1135,6 +1135,7 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) struct irq_desc *desc = data; unsigned int i, irq = desc - irq_desc; irq_guest_action_t *action; + cpumask_t *cpu_eoi_map; spin_lock_irq(&desc->lock); @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) switch ( action->ack_type ) { - cpumask_t *cpu_eoi_map; - case ACKTYPE_UNMASK: if ( desc->handler->end ) desc->handler->end(desc, 0); diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index ecf126566d..0e61e0fe4e 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -335,11 +335,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) const struct cpu_policy *cp = d->arch.cpu_policy; struct vcpu_msrs *msrs = v->arch.msrs; int ret = X86EMUL_OKAY; + uint64_t rsvd; switch ( msr ) { - uint64_t rsvd; - /* Read-only */ case MSR_IA32_PLATFORM_ID: case MSR_CORE_CAPABILITIES:
Variable declarations between a switch statement guard and before any case label are unreachable code, and hence violate Rule 2.1: "A project shall not contain unreachable code". Therefore the declarations are moved in the smallest enclosing scope, near other variable definitions. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/cpuid.c | 3 +-- xen/arch/x86/domain.c | 23 +++++++++++------------ xen/arch/x86/irq.c | 3 +-- xen/arch/x86/msr.c | 3 +-- 4 files changed, 14 insertions(+), 18 deletions(-)