Re: [PATCH] automation/eclair: deviate intentionally unreachable code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2025 16:06, Dmytro Prokopchuk1 wrote:
> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> 
> MISRA C Rule 2.1 states: "A project shall not contain unreachable code".
> Functions that are non-returning and are not explicitly annotated with
> the `noreturn' attribute are considered a violation of this rule.
> 
> In certain cases, some functions might be non-returning in debug build
> configuration (when `NDEBUG' is not defined), due to calls to
> `__builtin_unreachable' in the expansion of the macro `ASSERT_UNREACHABLE()'.
> 
> Conversely, in non-debug (release) builds (when `NDEBUG' is defined),
> the macro `ASSERT_UNREACHABLE()' expands to an empty construct
> (`do { } while (0)'), which does not affect the execution flow.
> This allows such functions to return normally in release builds,
> avoiding unreachable code.

While this way of putting it is technically correct as long as all we have
is

#ifndef CONFIG_DEBUG
#define NDEBUG
#endif

in xen/config.h, but I think it would be better if "debug builds"
(controlled by CONFIG_DEBUG) were properly separated from assertions
being active (NDEBUG). For quite some time there has been the plan to
decouple the two.

Similarly, throughout: You write ASSERT_UNREACHABLE() (i.e. including
the parentheses), yet oddly you then don't similarly include them when
referring to __builtin_unreachable().

> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -124,6 +124,15 @@ maintainers if you want to suggest a change.
>             they are used to generate definitions for asm modules
>           - Declarations without initializer are safe, as they are not
>             executed
> +         - Functions that are noreturn due to calls to `ASSERT_UNREACHABLE`
> +           macro in debug build configurations are not reported as 
> violations::

I find "reported" odd to appear in this file. My take is that here we
describe our intentions, not what some tool may be doing. I'd suggest
"deemed" as replacement.

Jan

> +              static inline bool
> +              arch_vcpu_ioreq_completion(enum vio_completion completion)
> +              {
> +                  ASSERT_UNREACHABLE();
> +                  return false;
> +              }
>  
>     * - `Rule 2.6 
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_06.c>`_
>       - Advisory