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

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

[Nicola Vetrini]

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(-)

-- 
2.34.1

[RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop

[Nicola Vetrini]

The backwards goto in the vsnprintf function can be replaced
with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/vsprintf.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index c49631c0a4d8..603bae44177a 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
     }
 
     for (; *fmt ; ++fmt) {
+        bool repeat = true;
+
         if (*fmt != '%') {
             if (str < end)
                 *str = *fmt;
@@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 
         /* process flags */
         flags = 0;
-    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;
+        while ( repeat ) {
+            ++fmt;          /* this also skips the first '%' */
+            switch (*fmt) {
+            case '-': flags |= LEFT; break;
+            case '+': flags |= PLUS; break;
+            case ' ': flags |= SPACE; break;
+            case '#': flags |= SPECIAL; break;
+            case '0': flags |= ZEROPAD; break;
+            default: repeat = false; break;
+            }
         }
 
         /* get field width */
-- 
2.34.1

Re: [RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop

[Andrew Cooper]

On 07/11/2023 10:33 am, Nicola Vetrini wrote:
> The backwards goto in the vsnprintf function can be replaced
> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/common/vsprintf.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index c49631c0a4d8..603bae44177a 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>      }
>  
>      for (; *fmt ; ++fmt) {
> +        bool repeat = true;
> +
>          if (*fmt != '%') {
>              if (str < end)
>                  *str = *fmt;
> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  
>          /* process flags */
>          flags = 0;
> -    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;
> +        while ( repeat ) {
> +            ++fmt;          /* this also skips the first '%' */
> +            switch (*fmt) {
> +            case '-': flags |= LEFT; break;
> +            case '+': flags |= PLUS; break;
> +            case ' ': flags |= SPACE; break;
> +            case '#': flags |= SPECIAL; break;
> +            case '0': flags |= ZEROPAD; break;
> +            default: repeat = false; break;
> +            }

I'm firmly against this change.  It takes a simple and clear piece of
code and replaces it with something harder to follow because you have to
look elsewhere to figure how the variable works.

Labels with names such as repeat/again/retry are clearly forming a
loop(ish).

I see in patch 4 that you exempt again/retry.  That list needs to
include repeat, and this patch wants dropping.

~Andrew

Re: [RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop

[Jan Beulich]

On 07.11.2023 12:36, Andrew Cooper wrote:
> On 07/11/2023 10:33 am, Nicola Vetrini wrote:
>> The backwards goto in the vsnprintf function can be replaced
>> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/common/vsprintf.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>> index c49631c0a4d8..603bae44177a 100644
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>      }
>>  
>>      for (; *fmt ; ++fmt) {
>> +        bool repeat = true;
>> +
>>          if (*fmt != '%') {
>>              if (str < end)
>>                  *str = *fmt;
>> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>  
>>          /* process flags */
>>          flags = 0;
>> -    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;
>> +        while ( repeat ) {
>> +            ++fmt;          /* this also skips the first '%' */
>> +            switch (*fmt) {
>> +            case '-': flags |= LEFT; break;
>> +            case '+': flags |= PLUS; break;
>> +            case ' ': flags |= SPACE; break;
>> +            case '#': flags |= SPECIAL; break;
>> +            case '0': flags |= ZEROPAD; break;
>> +            default: repeat = false; break;
>> +            }
> 
> I'm firmly against this change.  It takes a simple and clear piece of
> code and replaces it with something harder to follow because you have to
> look elsewhere to figure how the variable works.

While I don't really like that change either, I also don't like uses of
goto (at some point we said using it for error handling is okay, but
the case here is clearly not in that category). So at least for
consideration, how about getting away without a new variable:

        for ( ; ; )
        {
            ++fmt;          /* this also skips the first '%' */
            switch ( *fmt )
            {
            case '-': flags |= LEFT; continue;
            case '+': flags |= PLUS; continue;
            case ' ': flags |= SPACE; continue;
            case '#': flags |= SPECIAL; continue;
            case '0': flags |= ZEROPAD; continue;
            }
            break;
        }

Jan

[RFC PATCH 2/4] x86/dom0: make goto jump forward

[Nicola Vetrini]

The jump to the label 'parse_error' becomes forward, rather
than backward; at the same time, the else branch can be eliminated.

This also fixes a violation of MISRA C:2012 Rule 15.2.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/dom0_build.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 09fb8b063ae7..f0191dc148a2 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -439,12 +439,7 @@ static void __init process_dom0_ioports_disable(struct domain *dom0)
     {
         io_from = simple_strtoul(t, &u, 16);
         if ( u == t )
-        {
-        parse_error:
-            printk("Invalid ioport range <%s> "
-                   "in dom0_ioports_disable, skipping\n", t);
-            continue;
-        }
+            goto parse_error;
 
         if ( *u == '\0' )
             io_to = io_from;
@@ -454,7 +449,12 @@ static void __init process_dom0_ioports_disable(struct domain *dom0)
             goto parse_error;
 
         if ( (*u != '\0') || (io_to < io_from) || (io_to >= 65536) )
-            goto parse_error;
+        {
+        parse_error:
+            printk("Invalid ioport range <%s> "
+                   "in dom0_ioports_disable, skipping\n", t);
+            continue;
+        }
 
         printk("Disabling dom0 access to ioport range %04lx-%04lx\n",
             io_from, io_to);
-- 
2.34.1

Re: [RFC PATCH 2/4] x86/dom0: make goto jump forward

[Jan Beulich]

On 07.11.2023 11:33, Nicola Vetrini wrote:
> The jump to the label 'parse_error' becomes forward, rather
> than backward; at the same time, the else branch can be eliminated.

What "else branch" is this referring to?

> This also fixes a violation of MISRA C:2012 Rule 15.2.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/dom0_build.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 09fb8b063ae7..f0191dc148a2 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -439,12 +439,7 @@ static void __init process_dom0_ioports_disable(struct domain *dom0)
>      {
>          io_from = simple_strtoul(t, &u, 16);
>          if ( u == t )
> -        {
> -        parse_error:
> -            printk("Invalid ioport range <%s> "
> -                   "in dom0_ioports_disable, skipping\n", t);
> -            continue;
> -        }
> +            goto parse_error;
>  
>          if ( *u == '\0' )
>              io_to = io_from;
> @@ -454,7 +449,12 @@ static void __init process_dom0_ioports_disable(struct domain *dom0)
>              goto parse_error;
>  
>          if ( (*u != '\0') || (io_to < io_from) || (io_to >= 65536) )
> -            goto parse_error;
> +        {
> +        parse_error:
> +            printk("Invalid ioport range <%s> "
> +                   "in dom0_ioports_disable, skipping\n", t);
> +            continue;
> +        }
>  
>          printk("Disabling dom0 access to ioport range %04lx-%04lx\n",
>              io_from, io_to);

While purely from how the change looks I think I could live with this kind
of code adjustment (not a lot of churn, in particular no re-indentation,
no potentially harder to follow use of [new] local variables), this is
precisely one of the cases where I think having the label appear before
the goto is actually better, for matching how the language works otherwise
(things you want to use first need to be declared / defined).

Jan

[RFC PATCH 3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2

[Nicola Vetrini]

The backwards jump due to the "goto retry;" statement
can be transformed into a loop, without losing much in terms
of readability.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
This specific patch was provided by Stefano, I just added the
commit message.
---
 xen/arch/arm/gic-v3-its.c | 81 ++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 8afcd9783bc8..4ba3f869ddf2 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
     unsigned int pagesz = 2;    /* try 64K pages first, then go down. */
     unsigned int table_size;
     void *buffer;
+    bool retry = false;
 
     attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
     attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
@@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
      * it back and see what sticks (page size, cacheability and shareability
      * attributes), retrying if necessary.
      */
-retry:
-    table_size = ROUNDUP(nr_items * entry_size,
-                         BIT(BASER_PAGE_BITS(pagesz), UL));
-    /* The BASE registers support at most 256 pages. */
-    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
+    while ( retry )
+    {
+        table_size = ROUNDUP(nr_items * entry_size,
+                BIT(BASER_PAGE_BITS(pagesz), UL));
+        /* The BASE registers support at most 256 pages. */
+        table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
 
-    buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
-    if ( !buffer )
-        return -ENOMEM;
+        buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
+        if ( !buffer )
+            return -ENOMEM;
 
-    if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
-    {
-        xfree(buffer);
-        return -ERANGE;
-    }
+        if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
+        {
+            xfree(buffer);
+            return -ERANGE;
+        }
 
-    reg  = attr;
-    reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
-    reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
-    reg |= regc & BASER_RO_MASK;
-    reg |= GITS_VALID_BIT;
-    reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
-                                  BASER_PAGE_BITS(pagesz));
+        reg  = attr;
+        reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
+        reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
+        reg |= regc & BASER_RO_MASK;
+        reg |= GITS_VALID_BIT;
+        reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
+                BASER_PAGE_BITS(pagesz));
 
-    writeq_relaxed(reg, basereg);
-    regc = readq_relaxed(basereg);
+        writeq_relaxed(reg, basereg);
+        regc = readq_relaxed(basereg);
 
-    /* The host didn't like our attributes, just use what it returned. */
-    if ( (regc & BASER_ATTR_MASK) != attr )
-    {
-        /* If we can't map it shareable, drop cacheability as well. */
-        if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
+        /* The host didn't like our attributes, just use what it returned. */
+        if ( (regc & BASER_ATTR_MASK) != attr )
         {
-            regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
-            writeq_relaxed(regc, basereg);
+            /* If we can't map it shareable, drop cacheability as well. */
+            if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
+            {
+                regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
+                writeq_relaxed(regc, basereg);
+            }
+            attr = regc & BASER_ATTR_MASK;
         }
-        attr = regc & BASER_ATTR_MASK;
-    }
-    if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
-        clean_and_invalidate_dcache_va_range(buffer, table_size);
+        if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
+            clean_and_invalidate_dcache_va_range(buffer, table_size);
 
-    /* If the host accepted our page size, we are done. */
-    if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
-        return 0;
+        /* If the host accepted our page size, we are done. */
+        if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
+            return 0;
 
-    xfree(buffer);
+        xfree(buffer);
 
-    if ( pagesz-- > 0 )
-        goto retry;
+        if ( pagesz-- > 0 )
+            retry = true;
+    }
 
     /* None of the page sizes was accepted, give up */
     return -EINVAL;
-- 
2.34.1

Re: [RFC PATCH 3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2

[Julien Grall]

Hi Nicola,

On 07/11/2023 10:33, Nicola Vetrini wrote:
> The backwards jump due to the "goto retry;" statement
> can be transformed into a loop, without losing much in terms
> of readability.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> This specific patch was provided by Stefano, I just added the
> commit message.

If that's the case, then Stefano should be the Author (at the moment 
this is you).

> ---
>   xen/arch/arm/gic-v3-its.c | 81 ++++++++++++++++++++-------------------
>   1 file changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 8afcd9783bc8..4ba3f869ddf2 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
>       unsigned int pagesz = 2;    /* try 64K pages first, then go down. */
>       unsigned int table_size;
>       void *buffer;
> +    bool retry = false;

retry is false so...

>   
>       attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>       attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> @@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
>        * it back and see what sticks (page size, cacheability and shareability
>        * attributes), retrying if necessary.
>        */
> -retry:
> -    table_size = ROUNDUP(nr_items * entry_size,
> -                         BIT(BASER_PAGE_BITS(pagesz), UL));
> -    /* The BASE registers support at most 256 pages. */
> -    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
> +    while ( retry )

... you will never end in the loop. I also think that name 'retry' is 
confusing as the first time is not retry.

It would be clearer if we use a

do {

} while (retry)

That said, I actually prefer the goto version because some of the lines 
within the loop are now over 80 characters and splitting them would make 
the code harder to read.

Below an example, with the new indentation it is just over 80 characters.

if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )

One possibly would be to move the logic within the loop in a separate 
function.

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

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

[Julien Grall]

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

[Nicola Vetrini]

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

[Julien Grall]

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

[Nicola Vetrini]

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)

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

[Jan Beulich]

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

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

[Nicola Vetrini]

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, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Nicola Vetrini]

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.

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