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 <[email protected]>
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.
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
