Re: [PATCH 04/12] x86: add noreturn in a few more places

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2026 22:48, Nicola Vetrini wrote:
> 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.

Hmm, why would this be subtly broken? do_double_fault() doesn't (and won't
ever) return. So the compiler optimizing based on that is quite okay.

> 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).

This would feel odd to me - do_double_fault() is really already BUG()-like,
so following a call to it by such a construct would end up questionable.

All of the above (and maybe more) is what I understand makes Andrew say
"Eclair needs to be taught ...". Andrew, maybe you could expand a bit?

Jan