Re: [PATCH 1/3] xen/keyhandler: add missing noreturn attribute

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-06-05 14:22, Nicola Vetrini wrote:
> On 2025-06-05 09:17, Jan Beulich wrote:
> > On 05.06.2025 01:49, victorm.lira@xxxxxxx wrote:
> > > From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > >
> > > Function `reboot_machine' does not return, but lacks the `noreturn' 
> > > attribute,
> > > therefore causing a violation of MISRA C Rule 2.1: "A project shall 
> > > not contain
> > > unreachable code".
> > Is this (uniformly) true? Looking at ...
> >
> > > --- a/xen/common/keyhandler.c
> > > +++ b/xen/common/keyhandler.c
> > > @@ -251,7 +251,7 @@ static void cf_check 
> > > dump_hwdom_registers(unsigned char key)
> > >      }
> > >  }
> > >
> > > -static void cf_check reboot_machine(unsigned char key, bool unused)
> > > +static void noreturn cf_check reboot_machine(unsigned char key, bool 
> > > unused)
> > >  {
> > >      printk("'%c' pressed -> rebooting machine\n", key);
> > >      machine_restart(0);
> > ... generated code here, I can see that the compiler is perfectly able 
> > to
> > leverage the noreturn that machine_restart() has, resulting in no
> > unreachable code to be generated. That is - neither in source nor in
> > binary there is any unreachable code. Therefore I'm having a hard time
> > seeing what the violation is here.
> >
> > That said, I certainly don't mind the addition of the (seemingly) 
> > missing
> > attribute. Otoh I wonder whether an attribute the removal of which has 
> > no
> > effect wouldn't count as "dead code" or alike, violating some other 
> > rule.
> Inlining does not play a role in this case. Here reboot_machine() is 
> marked as a violation because machine_restart() is noreturn and there 
> is no other path upon which reboot_machine() may return, hence any 
> function calling reboot_machine() could have portions that are 
> inadvertently unreachable (as in never executed due to divergence) by 
> not having the annotation. That said, in such trivial cases compilers 
> are typically able to derive the property automatically, but they are 
> not obliged to and, more importantly, the behavior may even differ with 
> the same compiler using different optimization levels.
Just a note: in later revisions of MISRA C this has become a rule of its 
own [1], which helps reduce confusion, but up to MISRA C:2012 Amendment 
2 (currently used by Xen), this is part of Rule 2.1.
[1] Rule 17.11: "A function that never returns should be declared with a 
_Noreturn function specifier"
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253