mbox series

[RFC,0/4] address MISRA C:2012 Rule 15.2

Message ID cover.1699295113.git.nicola.vetrini@bugseng.com (mailing list archive)
Headers show
Series address MISRA C:2012 Rule 15.2 | expand

Message

Nicola Vetrini Nov. 7, 2023, 10:33 a.m. UTC
This series is aimed at presenting some strategies that can be used to deal with
violations of Rule 15.2:
"The goto statement shall jump to a label declared later in the same function".

The rule's rationale is about possible developer confusion, therefore it could
be argued that there is no substantial gain in complying with it, given the
torough review process in place.

Nonetheless, the proposed resolution strategies are the following:
- use a loop instead of a goto (see patch 1 and 3)
- make the jump due to the goto forward, rather than backward (see patch 2)
- unconditionally allow certain constructs, such as "goto retry", whose presence
  in the codebase typically signifies that all other reasonable approaches (e.g,
  loops, forward jumps) have been considered and deemed inferior in terms of
  code readability.
  
The latter strategy may be postponed until all goto-s with a certain label have
been examined. An alternative strategy could be to allow certain files
(most notably those under x86/x86_emulate) to have backward jumps, and resolve
the remaining violations.

Any feedback on this matter is welcome.

Nicola Vetrini (4):
  xen/vsprintf: replace backwards jump with loop
  x86/dom0: make goto jump forward
  xen/arm: GICv3: address MISRA C:2012 Rule 15.2
  automation/eclair: add deviation for certain backwards goto

 .../eclair_analysis/ECLAIR/deviations.ecl     | 10 +++
 docs/misra/deviations.rst                     | 10 +++
 xen/arch/arm/gic-v3-its.c                     | 81 ++++++++++---------
 xen/arch/x86/dom0_build.c                     | 14 ++--
 xen/common/vsprintf.c                         | 20 +++--
 5 files changed, 81 insertions(+), 54 deletions(-)

Comments

Jan Beulich Nov. 7, 2023, 10:52 a.m. UTC | #1
On 07.11.2023 11:33, Nicola Vetrini wrote:
> This series is aimed at presenting some strategies that can be used to deal with
> violations of Rule 15.2:
> "The goto statement shall jump to a label declared later in the same function".

I don't recall this rule being discussed on any of the meetings.

> The rule's rationale is about possible developer confusion, therefore it could
> be argued that there is no substantial gain in complying with it, given the
> torough review process in place.

To be honest, forward goto have potential of developer confusion as well: All
other entities need to be declared / defined before they can be used. Just
labels don't. (Or have I missed any other outlier?) IOW if I saw Misra make
any rule here, I think it ought to suggest to avoid using "goto" altogether.

Jan
Nicola Vetrini Nov. 7, 2023, 11:10 a.m. UTC | #2
On 2023-11-07 11:52, Jan Beulich wrote:
> On 07.11.2023 11:33, Nicola Vetrini wrote:
>> This series is aimed at presenting some strategies that can be used to 
>> deal with
>> violations of Rule 15.2:
>> "The goto statement shall jump to a label declared later in the same 
>> function".
> 
> I don't recall this rule being discussed on any of the meetings.
> 

This series is aimed mainly at collecting opinions on the possible 
resolution strategies,
and based on the feedback decide what to focus the discussion on in the 
meetings.

>> The rule's rationale is about possible developer confusion, therefore 
>> it could
>> be argued that there is no substantial gain in complying with it, 
>> given the
>> torough review process in place.
> 
> To be honest, forward goto have potential of developer confusion as 
> well: All
> other entities need to be declared / defined before they can be used. 
> Just
> labels don't. (Or have I missed any other outlier?) IOW if I saw Misra 
> make
> any rule here, I think it ought to suggest to avoid using "goto" 
> altogether.
> 
> Jan

There is Rule 15.1 that says precisely that "do not use goto", but it's 
advisory and has never been proposed
(there are likely hundreds of violations, and some are perhaps 
legitimate uses of goto).
MISRA says that, if 15.1 is not followed, then a constrained use of goto 
is regulated by subsequent
rules 15.2 and 15.3.
Nicola Vetrini Nov. 21, 2023, 2:41 p.m. UTC | #3
On 2023-11-07 11:33, Nicola Vetrini wrote:
> This series is aimed at presenting some strategies that can be used to 
> deal with
> violations of Rule 15.2:
> "The goto statement shall jump to a label declared later in the same 
> function".
> 
> The rule's rationale is about possible developer confusion, therefore 
> it could
> be argued that there is no substantial gain in complying with it, given 
> the
> torough review process in place.
> 
> Nonetheless, the proposed resolution strategies are the following:
> - use a loop instead of a goto (see patch 1 and 3)
> - make the jump due to the goto forward, rather than backward (see 
> patch 2)
> - unconditionally allow certain constructs, such as "goto retry", whose 
> presence
>   in the codebase typically signifies that all other reasonable 
> approaches (e.g,
>   loops, forward jumps) have been considered and deemed inferior in 
> terms of
>   code readability.
> 
> The latter strategy may be postponed until all goto-s with a certain 
> label have
> been examined. An alternative strategy could be to allow certain files
> (most notably those under x86/x86_emulate) to have backward jumps, and 
> resolve
> the remaining violations.
> 
> Any feedback on this matter is welcome.
> 
> Nicola Vetrini (4):
>   xen/vsprintf: replace backwards jump with loop
>   x86/dom0: make goto jump forward
>   xen/arm: GICv3: address MISRA C:2012 Rule 15.2
>   automation/eclair: add deviation for certain backwards goto
> 
>  .../eclair_analysis/ECLAIR/deviations.ecl     | 10 +++
>  docs/misra/deviations.rst                     | 10 +++
>  xen/arch/arm/gic-v3-its.c                     | 81 ++++++++++---------
>  xen/arch/x86/dom0_build.c                     | 14 ++--
>  xen/common/vsprintf.c                         | 20 +++--
>  5 files changed, 81 insertions(+), 54 deletions(-)

Cc: all involved maintainers

In concert with Stefano, and given the feedback received on this RFC 
series, it has been decided to deviate globally this rule, on the 
grounds that there are already guidelines on the usage of goto-s within 
Xen, so it was deemed not worth pursuing the resolution of these 
violations. Therefore, the patches contained here can be ignored for the 
purposes of MISRA compliance.