[XEN PATCH 1/2] automation/eclair: ECLAIR configuration changes due to GitLab runner update

[XEN PATCH 1/2] automation/eclair: ECLAIR configuration changes due to GitLab runner update

[Nicola Vetrini]

Update configurations to adapt to the new syntax used by the newer
version of the tool in the GitLab runner.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
This will of course need coordination with me to update the runners
just before committing the patches, as any analysis done in between will
fail due to syntax errors in the tool configuration files.
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index ebce1ceab912..ec0cac797e5f 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -536,8 +536,8 @@ safe."
 -config=MC3A2.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through\\.? \\*/.*$,0..2))))"}
 -doc_end
 
--doc_begin="Switch statements having a controlling expression of enum type deliberately do not have a default case: gcc -Wall enables -Wswitch which warns (and breaks the build as we use -Werror) if one of the enum labels is missing from the switch."
--config=MC3A2.R16.4,reports+={deliberate,'any_area(kind(context)&&^.* has no `default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,type(canonical(enum_underlying_type(any())))))))'}
+-doc_begin="Switch statements having a controlling expression of enum type deliberately do not have a default case: gcc -Wall enables -Wswitch which warns (and breaks the build as -Werror is used) if one of the enumeration values is missing from the switch."
+-config=MC3A2.R16.4,reports+={deliberate,"any_area(kind(context)&&^.* has no `default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,ref(enum_underlying_type(any()))))))"}
 -doc_end
 
 -doc_begin="A switch statement with a single switch clause and no default label may be used in place of an equivalent if statement if it is considered to improve readability."
@@ -596,10 +596,10 @@ in assignments; (5) as initializers, possibly designated, in initalizer lists;
 -config=MC3A2.R20.7,expansion_context=
 {safe, "context(__call_expr_arg_contexts)"},
 {safe, "left_right(^[(,\\[]$,^[),\\]]$)"},
-{safe, "context(skip_to(__expr_non_syntactic_contexts, stmt_child(node(array_subscript_expr), subscript)))"},
-{safe, "context(skip_to(__expr_non_syntactic_contexts, stmt_child(operator(assign), lhs)))"},
-{safe, "context(skip_to(__expr_non_syntactic_contexts, stmt_child(node(init_list_expr||designated_init_expr), init)))"},
-{safe, "context(skip_to(__expr_non_syntactic_contexts, stmt_child(node(case_stmt), lower||upper)))"}
+{safe, "context(skip(__expr_non_syntactic_contexts, is(subscript)&&node(array_subscript_expr)))"},
+{safe, "context(skip(__expr_non_syntactic_contexts, is(lhs)&&stmt(operator(assign))))"},
+{safe, "context(skip(__expr_non_syntactic_contexts, is(init)&&node(init_list_expr||designated_init_expr)))"},
+{safe, "context(skip(__expr_non_syntactic_contexts, is(lower||upper)&&node(case_stmt)))"}
 -doc_end
 
 -doc_begin="Violations involving the __config_enabled macros cannot be fixed without
-- 
2.43.0

[XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Nicola Vetrini]

The rule states: "Initializer lists shall not contain persistent side effects".
The specific way in which the 'mrs' instruction is used does not lead to
visible side effects for the surrounding code.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Not yet tested on the Xen ECLAIR runner, as the syntax used in the deviation
is only supported after updating the runner.

What the tool is reporting is that due to the '=r' constraint and the
semantics of the instruction, there is the side effect of writing to '_r',
but this is not observable outside the stmt expr. The deviation ends up being
a bit too general for my taste, but the restriction on the actual istruction
should be enough to limit applicability to cases that are arguably safe in
practice.

An alternative approach would be represented by stating that side effects in
'READ_SYSREG64' are safe, but this is not true in general.
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index ec0cac797e5f..6b492e38505d 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -437,6 +437,10 @@ write or not"
 # Series 13
 #
 
+-doc_begin="Consider the asm instruction to read an Arm system register to have no side effects."
+-asm_properties+={"asm(any())&&child(text, ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
+-doc_end
+
 -doc_begin="All developers and reviewers can be safely assumed to be well aware
 of the short-circuit evaluation strategy of such logical operators."
 -config=MC3A2.R13.5,reports+={disapplied,"any()"}
-- 
2.43.0

Re: [XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Dmytro Prokopchuk1]

On 8/9/25 00:40, Nicola Vetrini wrote:
> The rule states: "Initializer lists shall not contain persistent side effects".
> The specific way in which the 'mrs' instruction is used does not lead to
> visible side effects for the surrounding code.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Not yet tested on the Xen ECLAIR runner, as the syntax used in the deviation
> is only supported after updating the runner.
> 
> What the tool is reporting is that due to the '=r' constraint and the
> semantics of the instruction, there is the side effect of writing to '_r',
> but this is not observable outside the stmt expr. The deviation ends up being
> a bit too general for my taste, but the restriction on the actual istruction
> should be enough to limit applicability to cases that are arguably safe in
> practice.
> 
> An alternative approach would be represented by stating that side effects in
> 'READ_SYSREG64' are safe, but this is not true in general.
> ---
>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index ec0cac797e5f..6b492e38505d 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -437,6 +437,10 @@ write or not"
>   # Series 13
>   #
>   
> +-doc_begin="Consider the asm instruction to read an Arm system register to have no side effects."
> +-asm_properties+={"asm(any())&&child(text, ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
> +-doc_end
> +
>   -doc_begin="All developers and reviewers can be safely assumed to be well aware
>   of the short-circuit evaluation strategy of such logical operators."
>   -config=MC3A2.R13.5,reports+={disapplied,"any()"}

I think it's worth to add example of macro expansion in the commit 
description or asm_properties doc:

uint64_t _r; asm volatile("mrs  %0, ""TPIDR_EL2" : "=r" (_r));

This uses the 'mrs' instruction to read from the TPIDR_EL2 register. 
While this read operation accesses a system register, reading itself 
doesn't cause any persistent side effects, as no program state is modified.

Dmytro

Re: [XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Nicola Vetrini]

On 2025-08-13 09:41, Dmytro Prokopchuk1 wrote:
> On 8/9/25 00:40, Nicola Vetrini wrote:
>> The rule states: "Initializer lists shall not contain persistent side 
>> effects".
>> The specific way in which the 'mrs' instruction is used does not lead 
>> to
>> visible side effects for the surrounding code.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Not yet tested on the Xen ECLAIR runner, as the syntax used in the 
>> deviation
>> is only supported after updating the runner.
>> 
>> What the tool is reporting is that due to the '=r' constraint and the
>> semantics of the instruction, there is the side effect of writing to 
>> '_r',
>> but this is not observable outside the stmt expr. The deviation ends 
>> up being
>> a bit too general for my taste, but the restriction on the actual 
>> istruction
>> should be enough to limit applicability to cases that are arguably 
>> safe in
>> practice.
>> 
>> An alternative approach would be represented by stating that side 
>> effects in
>> 'READ_SYSREG64' are safe, but this is not true in general.
>> ---
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index ec0cac797e5f..6b492e38505d 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -437,6 +437,10 @@ write or not"
>>   # Series 13
>>   #
>> 
>> +-doc_begin="Consider the asm instruction to read an Arm system 
>> register to have no side effects."
>> +-asm_properties+={"asm(any())&&child(text, 
>> ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
>> +-doc_end
>> +
>>   -doc_begin="All developers and reviewers can be safely assumed to be 
>> well aware
>>   of the short-circuit evaluation strategy of such logical operators."
>>   -config=MC3A2.R13.5,reports+={disapplied,"any()"}
> 
> I think it's worth to add example of macro expansion in the commit
> description or asm_properties doc:
> 
> uint64_t _r; asm volatile("mrs  %0, ""TPIDR_EL2" : "=r" (_r));
> 
> This uses the 'mrs' instruction to read from the TPIDR_EL2 register.
> While this read operation accesses a system register, reading itself
> doesn't cause any persistent side effects, as no program state is 
> modified.
> 

Definitely not in the -doc_begin, perhaps in deviations.rst, though in 
reality it is a single case this currently applies to. Reading the 
register is not the reason why this deviation was requested, but the 
write with the "=r" constraint on "_r", as that is the side effect the 
tool is pointing at.

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Jan Beulich]

On 08.08.2025 23:40, Nicola Vetrini wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -437,6 +437,10 @@ write or not"
>  # Series 13
>  #
>  
> +-doc_begin="Consider the asm instruction to read an Arm system register to have no side effects."
> +-asm_properties+={"asm(any())&&child(text, ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
> +-doc_end

Is this actually strict enough to not allow multiple instructions in the asm(),
where some of the others would yield side effects we actually need to respect?

Jan

Re: [XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Nicola Vetrini]

On 2025-08-14 09:36, Jan Beulich wrote:
> On 08.08.2025 23:40, Nicola Vetrini wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -437,6 +437,10 @@ write or not"
>>  # Series 13
>>  #
>> 
>> +-doc_begin="Consider the asm instruction to read an Arm system 
>> register to have no side effects."
>> +-asm_properties+={"asm(any())&&child(text, 
>> ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
>> +-doc_end
> 
> Is this actually strict enough to not allow multiple instructions in 
> the asm(),
> where some of the others would yield side effects we actually need to 
> respect?
> 
> Jan

Good point, I didn't think of that. As it is, there is only one MRS asm, 
so no worries here, but in general the remark is valid. The regex could 
be a bit stricter, though the real solution here would be to match the 
context in which this asm is present to limit the scope of the 
deviation. This does not make much sense for an asm property because the 
properties of an asm statement (at the level of granularity targeted by 
ECLAIR) can be derived mostly in isolation from the surrounding code.

Another improvement we are pursuing is allowing single effects in 
init-list-exprs, but it will take a bit of time to surface (i.e., next 
major release probably).

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [XEN PATCH 2/2] Address violation of MISRA C Rule 13.1 involving asm side effects.

[Stefano Stabellini]

On Fri, 8 Aug 2025, Nicola Vetrini wrote:
> The rule states: "Initializer lists shall not contain persistent side effects".
> The specific way in which the 'mrs' instruction is used does not lead to
> visible side effects for the surrounding code.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Ideally, with the improved doc as suggested by Dmytro.


> ---
> Not yet tested on the Xen ECLAIR runner, as the syntax used in the deviation
> is only supported after updating the runner.
> 
> What the tool is reporting is that due to the '=r' constraint and the
> semantics of the instruction, there is the side effect of writing to '_r',
> but this is not observable outside the stmt expr. The deviation ends up being
> a bit too general for my taste, but the restriction on the actual istruction
> should be enough to limit applicability to cases that are arguably safe in
> practice.
> 
> An alternative approach would be represented by stating that side effects in
> 'READ_SYSREG64' are safe, but this is not true in general.
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index ec0cac797e5f..6b492e38505d 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -437,6 +437,10 @@ write or not"
>  # Series 13
>  #
>  
> +-doc_begin="Consider the asm instruction to read an Arm system register to have no side effects."
> +-asm_properties+={"asm(any())&&child(text, ast_field(value,^mrs\\s+%0.*$))", {no_side_effect}}
> +-doc_end
> +
>  -doc_begin="All developers and reviewers can be safely assumed to be well aware
>  of the short-circuit evaluation strategy of such logical operators."
>  -config=MC3A2.R13.5,reports+={disapplied,"any()"}
> -- 
> 2.43.0
> 

Re: [XEN PATCH 1/2] automation/eclair: ECLAIR configuration changes due to GitLab runner update

[Andrew Cooper]

On 08/08/2025 10:40 pm, Nicola Vetrini wrote:
> Update configurations to adapt to the new syntax used by the newer
> version of the tool in the GitLab runner.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> This will of course need coordination with me to update the runners
> just before committing the patches, as any analysis done in between will
> fail due to syntax errors in the tool configuration files.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [XEN PATCH 1/2] automation/eclair: ECLAIR configuration changes due to GitLab runner update

[Nicola Vetrini]

On 2025-08-08 23:40, Nicola Vetrini wrote:
> Update configurations to adapt to the new syntax used by the newer
> version of the tool in the GitLab runner.
> 

Sorry, forgot to include the cover letter. See below.

 From c4da2554d94c3f7bd4b1e460dcaa27db382ece2f Mon Sep 17 00:00:00 2001
Message-ID: <cover.1754689388.git.nicola.vetrini@bugseng.com>
 From: Nicola Vetrini <nicola.vetrini@bugseng.com>
Date: Fri, 8 Aug 2025 23:43:08 +0200
Subject: [XEN PATCH 0/2] Update ECLAIR runner and address MISRA 
violations

The first patch of the series contains the necessary updates to the
ECLAIR syntax needed to run the updated runners. The change was
motivated by usage of the new syntax in a deviation that is part of the
second patch.

As a result, the runners should be updated either just before or just
after committing the patches.

These have not been properly tested yet on an ECLAIR analysis of Xen, 
but
I sent the patches to get some feedback first and then coordinate with
maintainers to take the needed action once agreed on the approach.

Nicola Vetrini (2):
   automation/eclair: ECLAIR configuration changes due to GitLab runner
     update
   Address violation of MISRA C Rule 13.1 involving asm side effects.

  automation/eclair_analysis/ECLAIR/deviations.ecl | 16 ++++++++++------
  1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.43.0


> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> This will of course need coordination with me to update the runners
> just before committing the patches, as any analysis done in between 
> will
> fail due to syntax errors in the tool configuration files.
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index ebce1ceab912..ec0cac797e5f 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -536,8 +536,8 @@ safe."
>  -config=MC3A2.R16.3,reports+={safe, 
> "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through\\.? 
> \\*/.*$,0..2))))"}
>  -doc_end
> 
> --doc_begin="Switch statements having a controlling expression of enum 
> type deliberately do not have a default case: gcc -Wall enables 
> -Wswitch which warns (and breaks the build as we use -Werror) if one of 
> the enum labels is missing from the switch."
> --config=MC3A2.R16.4,reports+={deliberate,'any_area(kind(context)&&^.* 
> has no 
> `default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,type(canonical(enum_underlying_type(any())))))))'}
> +-doc_begin="Switch statements having a controlling expression of enum 
> type deliberately do not have a default case: gcc -Wall enables 
> -Wswitch which warns (and breaks the build as -Werror is used) if one 
> of the enumeration values is missing from the switch."
> +-config=MC3A2.R16.4,reports+={deliberate,"any_area(kind(context)&&^.* 
> has no 
> `default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,ref(enum_underlying_type(any()))))))"}
>  -doc_end
> 
>  -doc_begin="A switch statement with a single switch clause and no 
> default label may be used in place of an equivalent if statement if it 
> is considered to improve readability."
> @@ -596,10 +596,10 @@ in assignments; (5) as initializers, possibly 
> designated, in initalizer lists;
>  -config=MC3A2.R20.7,expansion_context=
>  {safe, "context(__call_expr_arg_contexts)"},
>  {safe, "left_right(^[(,\\[]$,^[),\\]]$)"},
> -{safe, "context(skip_to(__expr_non_syntactic_contexts, 
> stmt_child(node(array_subscript_expr), subscript)))"},
> -{safe, "context(skip_to(__expr_non_syntactic_contexts, 
> stmt_child(operator(assign), lhs)))"},
> -{safe, "context(skip_to(__expr_non_syntactic_contexts, 
> stmt_child(node(init_list_expr||designated_init_expr), init)))"},
> -{safe, "context(skip_to(__expr_non_syntactic_contexts, 
> stmt_child(node(case_stmt), lower||upper)))"}
> +{safe, "context(skip(__expr_non_syntactic_contexts, 
> is(subscript)&&node(array_subscript_expr)))"},
> +{safe, "context(skip(__expr_non_syntactic_contexts, 
> is(lhs)&&stmt(operator(assign))))"},
> +{safe, "context(skip(__expr_non_syntactic_contexts, 
> is(init)&&node(init_list_expr||designated_init_expr)))"},
> +{safe, "context(skip(__expr_non_syntactic_contexts, 
> is(lower||upper)&&node(case_stmt)))"}
>  -doc_end
> 
>  -doc_begin="Violations involving the __config_enabled macros cannot be 
> fixed without

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253