On 2025-08-05 15:22, Nicola Vetrini wrote:
On 2025-08-05 13:49, Dmytro Prokopchuk1 wrote:
On 7/31/25 19:09, Nicola Vetrini wrote:
On 2025-07-31 18:05, Andrew Cooper wrote:
On 31/07/2025 4:58 pm, Jan Beulich wrote:
On 31.07.2025 17:37, Andrew Cooper wrote:
On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote:
MISRA Rule 13.1: Initializer lists shall not contain persistent
side
effects.
The violations occur because both the `GVA_INFO` and `TRACE_TIME`
macro
expansions include expressions with persistent side effects
introduced
via inline assembly.
In the case of `GVA_INFO`, the issue stems from the initializer
list
containing a direct call to `current`, which evaluates to
`this_cpu(curr_vcpu)` and involves persistent side effects via
the
`asm` statement. To resolve this, the side-effect-producing
expression
is computed in a separate statement prior to the macro
initialization:
struct vcpu *current_vcpu = current;
The computed value is passed into the `GVA_INFO(current_vcpu)`
macro,
ensuring that the initializer is clean and free of such side
effects.
Similarly, the `TRACE_TIME` macro violates this rule when
accessing
expressions like `current->vcpu_id` and
`current->domain->domain_id`,
which also depend on `current` and inline assembly. To fix this,
the
value of `current` is assigned to a temporary variable:
struct vcpu *v = current;
This temporary variable is then used to access `domain_id` and
`vcpu_id`.
This ensures that the arguments passed to the `TRACE_TIME` macro
are
simple expressions free of persistent side effects.
Signed-off-by: Dmytro Prokopchuk <dmytro_prokopch...@epam.com>
The macro `current` specifically does not (and must not) have side
effects. It is expected to behave like a plain `struct vcpu
*current;`
variable, and what Eclair is noticing is the thread-local
machinery
under this_cpu() (or in x86's case, get_current()).
In ARM's case, it's literally reading the hardware thread pointer
register. Can anything be done to tell Eclair that `this_cpu()`
specifically does not have side effects?
The only reason that GVA_INFO() and TRACE_TIME() are picked out is
because they both contain embedded structure initialisation, and
this is
is actually an example where trying to comply with MISRA
interferes
with
what is otherwise a standard pattern in Xen.
Irrespective of what you say, some of the changes here were
eliminating
multiple adjacent uses of current, which - iirc - often the
compiler
can't fold via CSE.
Where we have mixed usage, sure. (I'm sure I've got a branch
somewhere
trying to add some more pure/const around to try and help out here,
but
I can't find it, and don't recall it being a major improvement
either.)
The real problem here is that there are a *very few* number of
contexts
where Eclair refuses to tolerate the use of `current` citing side
effects, despite there being no side effects.
That is the thing that breaks the principle of least surprise, and
we
ought to fix it by making Eclair happy with `current` everywhere,
rather
than force people to learn that 2 macros can't have a `current` in
their
parameter list.
I'll take a look. Likely yes, by adding a handful of properties.
There
are subtleties, though.
Hi, Nicola.
Did you have a chance to try configure Eclair to ignore this macro
`this_cpu()`?
Hi Dmytro,
I'm on it, I needed to handle other tasks first.
A solution has been devised by extending ECLAIR. The runner will be
updated with the latest ECLAIR version, and as a result a couple of
other patches will be submitted to adapt for it.
Thanks.
Dmytro
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
