On 2026-09-10 08:43, Jan Beulich wrote:
On 09.09.2026 21:07, Nicola Vetrini wrote:
On 2026-09-01 08:26, Jan Beulich wrote:
On 31.08.2026 21:13, Andrew Cooper wrote:
On 28/08/2026 8:01 am, Jan Beulich wrote:
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2304,7 +2304,7 @@ void asmlinkage entry_from_pv(struct cpu
case X86_ET_HW_EXC:
switch ( vec )
{
- case X86_EXC_DF: return do_double_fault(regs);
+ case X86_EXC_DF: do_double_fault(regs); /* noreturn */
case X86_EXC_MC: return do_machine_check(regs);
}
break;
@@ -2615,7 +2615,7 @@ void asmlinkage entry_from_xen(struct cp
case X86_ET_HW_EXC:
switch ( regs->fred_ss.vector )
{
- case X86_EXC_DF: return do_double_fault(regs);
+ case X86_EXC_DF: do_double_fault(regs); /* noreturn */
case X86_EXC_MC: return do_machine_check(regs);
}
break;
For starters you're missing a break, and the only reason this isn't
a
compile error is the trailing comment.
"break" there would again be unreachable, though.
Second, it's a tailcall anyway.
There really is nothing unreachable anywhere in this construct.
Just that the concept of "tailcall" is an optimization, not something
inherent to the language.
But by far the most important, it the singular noreturn attribute on
do_double_fault() (elsewhere, and not visible when reading these two
functions) which is preventing #DF falling into #MC. This
introduces
fragility which did not exist previously.
I realized that when making the patch, yet what do you do when the
rule
is as it is? Hence why I added the comment, really.
do_double_fault() would conditionally return if we ever got around
to
fixing espfix64.
And hence would have to lose its "noreturn". At which point call
sites
would need inspecting. (As said - yes, I do realize the fragility.)
So no - I'm going to insist that Eclair is taught to accept "return
some_noreturn_fn();" as intentional. It is objectively less fragile
than the MISRA-preferred option.
Nicola, thoughts?
If you find a suitable argument from the toolchain that the generated
code is correct even though you return from a function where you
promised not to return in its declaration, I suppose that's fine, but
that MISRA Rule I mentioned ("A function declared with a _Noreturn
function specifier shall not return to its caller"), which is not
(yet)
applied to Xen exists to defend from stumbling on UB 71 of C11: A
function declared with a _Noreturn function specifier shall not return
to its caller.
So in general ECLAIR should not accept this by default. What you can
do
is deviate these (hopefully few) cases if you have backing evidence of
the correct behavior.
The disagreement between you suggesting a deviation and Andrew
demanding
"that Eclair is taught to accept ..." will need resolving. The argument
towards the code being overall less fragile in its original shape
cannot
easily be put away. And Misra demanding code to be made more fragile
than it needs to be cannot really be the goal either.
Well, I feel like I have explained my reasoning, but let me step back a
bit and lay out the possible safe alternatives I see for this construct.
By the way, perhaps it's a better idea to split off this change from the
other additions of noreturn, which can probably go in as is.
Adding noreturn to do_double_fault() while keeping "return
do_double_fault()" in the #DF path is likely subtly broken (i.e. the
compiler can rightfully optimize assuming the function does not return)
so that's not a feasible solution.
If noreturn is added to do_double_fault(), removing the return in
entry_from_pv(), shouldn't that be guarded against falling trough via
BUG() or equivalent constructs that do not vanish in release builds? My
understanding, that may be incorrect, is that returning from
do_double_fault() is currently not expected to happen (hence the panic()
in it).
The third option is to ignore all this and not add noreturn to
do_double_fault, adding a specific deviation. The deviation is not
necessarily done via SAF, can also be something as shown below
(untested):
-config=MC3A2.R2.1,reports+={deliberate,
"any_area(decl(name(do_double_fault)))"}
and then in its documentation in rst you can summarize why it's not
being touched.
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
