1. Patchew
  2. Xen
  3. [RFC PATCH 0/4] address MISRA C:2012 Rule 15.2
  4. View patch

[RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto

Nicola Vetrini posted 4 patches 2 years, 3 months ago
[RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Posted by Nicola Vetrini 2 years, 3 months ago 
As explained in the deviation record, code constructs such as
"goto retry" and "goto again" are sometimes the best balance between
code complexity and the understandability of the control flow
by developers; as such, these construct are allowed to deviate
from Rule 15.2.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++++++++++
 docs/misra/deviations.rst                        | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..8b1f622f8f82 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -270,6 +270,16 @@ statements are deliberate"
 -config=MC3R1.R14.3,statements={deliberate , "wrapped(any(),node(if_stmt))" }
 -doc_end
 
+#
+# Series 15
+#
+
+-doc_begin="The additional complexity introduced in the code by using control flow structures other than backwards goto-s
+were deemed not to justify the possible prevention of developer confusion, given the very torough review process estabilished
+in the community."
+-config=MC3R1.R15.2,reports+={deliberate, "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
+-doc_end
+
 #
 # Series 20.
 #
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..7d1e1f0d09b3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
        statements are deliberate.
      - Project-wide deviation; tagged as `disapplied` for ECLAIR.
 
+   * - R15.2
+     - The possible prevention of developer confusion as a result of using
+       control flow structures other than backwards goto-s in some instances was
+       deemed not strong enough to justify the additional complexity introduced
+       in the code. Such instances are the uses of the following labels:
+       
+       - again
+       - retry
+     - Tagged as `deliberate` for ECLAIR.
+
    * - R20.7
      - Code violating Rule 20.7 is safe when macro parameters are used:
        (1) as function arguments;
-- 
2.34.1
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Posted by Julien Grall 2 years, 3 months ago 
Hi Nicola,

On 07/11/2023 10:33, Nicola Vetrini wrote:
> As explained in the deviation record, code constructs such as
> "goto retry" and "goto again" are sometimes the best balance between
> code complexity and the understandability of the control flow
> by developers; as such, these construct are allowed to deviate
> from Rule 15.2.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++++++++++
>   docs/misra/deviations.rst                        | 10 ++++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index fa56e5c00a27..8b1f622f8f82 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -270,6 +270,16 @@ statements are deliberate"
>   -config=MC3R1.R14.3,statements={deliberate , "wrapped(any(),node(if_stmt))" }
>   -doc_end
>   
> +#
> +# Series 15
> +#
> +
> +-doc_begin="The additional complexity introduced in the code by using control flow structures other than backwards goto-s
> +were deemed not to justify the possible prevention of developer confusion, given the very torough review process estabilished

Typoes: s/torough/thorough/ s/estabilished/established/

> +in the community."
> +-config=MC3R1.R15.2,reports+={deliberate, "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
> +-doc_end
> +
>   #
>   # Series 20.
>   #
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index 8511a189253b..7d1e1f0d09b3 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
>          statements are deliberate.
>        - Project-wide deviation; tagged as `disapplied` for ECLAIR.
>   
> +   * - R15.2
> +     - The possible prevention of developer confusion as a result of using
> +       control flow structures other than backwards goto-s in some instances was
> +       deemed not strong enough to justify the additional complexity introduced
> +       in the code. Such instances are the uses of the following labels:
> +
> +       - again
> +       - retry

Have you investigated the possibility to use SAF-X in the code? If so, 
what's the problem to use them?

Cheers,

-- 
Julien Grall
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Posted by Nicola Vetrini 2 years, 3 months ago 
Hi Julien,

On 2023-11-07 13:44, Julien Grall wrote:
> Hi Nicola,
> 
> On 07/11/2023 10:33, Nicola Vetrini wrote:
>> As explained in the deviation record, code constructs such as
>> "goto retry" and "goto again" are sometimes the best balance between
>> code complexity and the understandability of the control flow
>> by developers; as such, these construct are allowed to deviate
>> from Rule 15.2.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++++++++++
>>   docs/misra/deviations.rst                        | 10 ++++++++++
>>   2 files changed, 20 insertions(+)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index fa56e5c00a27..8b1f622f8f82 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -270,6 +270,16 @@ statements are deliberate"
>>   -config=MC3R1.R14.3,statements={deliberate , 
>> "wrapped(any(),node(if_stmt))" }
>>   -doc_end
>>   +#
>> +# Series 15
>> +#
>> +
>> +-doc_begin="The additional complexity introduced in the code by using 
>> control flow structures other than backwards goto-s
>> +were deemed not to justify the possible prevention of developer 
>> confusion, given the very torough review process estabilished
> 
> Typoes: s/torough/thorough/ s/estabilished/established/
> 

Thanks

>> +in the community."
>> +-config=MC3R1.R15.2,reports+={deliberate, 
>> "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
>> +-doc_end
>> +
>>   #
>>   # Series 20.
>>   #
>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>> index 8511a189253b..7d1e1f0d09b3 100644
>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
>>          statements are deliberate.
>>        - Project-wide deviation; tagged as `disapplied` for ECLAIR.
>>   +   * - R15.2
>> +     - The possible prevention of developer confusion as a result of 
>> using
>> +       control flow structures other than backwards goto-s in some 
>> instances was
>> +       deemed not strong enough to justify the additional complexity 
>> introduced
>> +       in the code. Such instances are the uses of the following 
>> labels:
>> +
>> +       - again
>> +       - retry
> 
> Have you investigated the possibility to use SAF-X in the code? If so, 
> what's the problem to use them?
> 
> Cheers,

This is another viable option: putting the SAF comment on top of the 
label should suffice,
as shown below:

/* SAF-2-safe */
repeat:
     ++fmt;          /* this also skips first '%' */
     switch (*fmt) {
     case '-': flags |= LEFT; goto repeat;
     case '+': flags |= PLUS; goto repeat;
     case ' ': flags |= SPACE; goto repeat;
     case '#': flags |= SPECIAL; goto repeat;
     case '0': flags |= ZEROPAD; goto repeat;
     }

I think it ultimately boils down to whether Xen wants to promote the use 
of certain labels
as the designated alternative when no other control flow mechanism is 
clearer from a
readability perspective (in which case there should be a consistent 
naming to capture and deviate
all of them, such as "retry") or do so on a case-by-case basis with a 
SAF, which is ok, but then
it needs someone to check each one and either fix them or mark them as 
ok.
Yet another route could be to mark with a SAF all those present right 
now to establish a baseline.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Posted by Julien Grall 2 years, 3 months ago 
On 07/11/2023 14:45, Nicola Vetrini wrote:
> Hi Julien,

Hi,

> On 2023-11-07 13:44, Julien Grall wrote:
>>> +in the community."
>>> +-config=MC3R1.R15.2,reports+={deliberate, 
>>> "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
>>> +-doc_end
>>> +
>>>   #
>>>   # Series 20.
>>>   #
>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>> index 8511a189253b..7d1e1f0d09b3 100644
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
>>>          statements are deliberate.
>>>        - Project-wide deviation; tagged as `disapplied` for ECLAIR.
>>>   +   * - R15.2
>>> +     - The possible prevention of developer confusion as a result of 
>>> using
>>> +       control flow structures other than backwards goto-s in some 
>>> instances was
>>> +       deemed not strong enough to justify the additional complexity 
>>> introduced
>>> +       in the code. Such instances are the uses of the following 
>>> labels:
>>> +
>>> +       - again
>>> +       - retry
>>
>> Have you investigated the possibility to use SAF-X in the code? If so, 
>> what's the problem to use them?
>>
>> Cheers,
> 
> This is another viable option: putting the SAF comment on top of the 
> label should suffice,
> as shown below:
> 
> /* SAF-2-safe */
> repeat:
>      ++fmt;          /* this also skips first '%' */
>      switch (*fmt) {
>      case '-': flags |= LEFT; goto repeat;
>      case '+': flags |= PLUS; goto repeat;
>      case ' ': flags |= SPACE; goto repeat;
>      case '#': flags |= SPECIAL; goto repeat;
>      case '0': flags |= ZEROPAD; goto repeat;
>      }
> 
> I think it ultimately boils down to whether Xen wants to promote the use 
> of certain labels
> as the designated alternative when no other control flow mechanism is 
> clearer from a
> readability perspective (in which case there should be a consistent 
> naming to capture and deviate
> all of them, such as "retry") or do so on a case-by-case basis with a 
> SAF, which is ok,

I would prefer a case-by-case basis because it adds an additional 
review. With deviating by keywords, the reviewrs/developpers may not be 
aware of the deviation (which may be fine for some, but IMHO not this one).

> but then
> it needs someone to check each one and either fix them or mark them as ok.

Don't we technically already need to go through all the existing use of 
ready & co even if we deviate by keyword?

> Yet another route could be to mark with a SAF all those present right 
> now to establish a baseline.

How many do we have?

Cheers,

-- 
Julien Grall
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Posted by Nicola Vetrini 2 years, 3 months ago 
On 2023-11-07 18:35, Julien Grall wrote:
> On 07/11/2023 14:45, Nicola Vetrini wrote:
>> Hi Julien,
> 
> Hi,
> 
>> On 2023-11-07 13:44, Julien Grall wrote:
>>>> +in the community."
>>>> +-config=MC3R1.R15.2,reports+={deliberate, 
>>>> "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
>>>> +-doc_end
>>>> +
>>>>   #
>>>>   # Series 20.
>>>>   #
>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>> index 8511a189253b..7d1e1f0d09b3 100644
>>>> --- a/docs/misra/deviations.rst
>>>> +++ b/docs/misra/deviations.rst
>>>> @@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
>>>>          statements are deliberate.
>>>>        - Project-wide deviation; tagged as `disapplied` for ECLAIR.
>>>>   +   * - R15.2
>>>> +     - The possible prevention of developer confusion as a result 
>>>> of using
>>>> +       control flow structures other than backwards goto-s in some 
>>>> instances was
>>>> +       deemed not strong enough to justify the additional 
>>>> complexity introduced
>>>> +       in the code. Such instances are the uses of the following 
>>>> labels:
>>>> +
>>>> +       - again
>>>> +       - retry
>>> 
>>> Have you investigated the possibility to use SAF-X in the code? If 
>>> so, what's the problem to use them?
>>> 
>>> Cheers,
>> 
>> This is another viable option: putting the SAF comment on top of the 
>> label should suffice,
>> as shown below:
>> 
>> /* SAF-2-safe */
>> repeat:
>>      ++fmt;          /* this also skips first '%' */
>>      switch (*fmt) {
>>      case '-': flags |= LEFT; goto repeat;
>>      case '+': flags |= PLUS; goto repeat;
>>      case ' ': flags |= SPACE; goto repeat;
>>      case '#': flags |= SPECIAL; goto repeat;
>>      case '0': flags |= ZEROPAD; goto repeat;
>>      }
>> 
>> I think it ultimately boils down to whether Xen wants to promote the 
>> use of certain labels
>> as the designated alternative when no other control flow mechanism is 
>> clearer from a
>> readability perspective (in which case there should be a consistent 
>> naming to capture and deviate
>> all of them, such as "retry") or do so on a case-by-case basis with a 
>> SAF, which is ok,
> 
> I would prefer a case-by-case basis because it adds an additional 
> review. With deviating by keywords, the reviewrs/developpers may not be 
> aware of the deviation (which may be fine for some, but IMHO not this 
> one).
> 

Ok, I'll keep this in mind when the rule will be discussed.

>> but then
>> it needs someone to check each one and either fix them or mark them as 
>> ok.
> 
> Don't we technically already need to go through all the existing use of 
> ready & co even if we deviate by keyword?
> 

my hope was trying to extract a common well-known pattern that can be
defensible as a deviation and then fix the rest.

>> Yet another route could be to mark with a SAF all those present right 
>> now to establish a baseline.
> 
> How many do we have?
> 

~30 in Arm (half of which are in common code) and ~250 in x86. The 
actual count of labels
is lower, because a report is given for each use of a backward jump, but 
those under
x86e_emulate likely do not (~40 total on x86 remain if we exclude 
x86_emulate/.*).

https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set3/372/PROJECT.ecd;/by_service/MC3R1.R15.2.html

https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/x86_64/staging/X86_64-Set3/372/PROJECT.ecd;/by_service/MC3R1.R15.2.html#

> Cheers,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.