Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

From: Stefano Stabellini <sstabellini@kernel.org>
To: Nicola Vetrini <nicola.vetrini@bugseng.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	 xen-devel@lists.xenproject.org, consulting@bugseng.com,
	 Bertrand Marquis <bertrand.marquis@arm.com>,
	 Michal Orzel <michal.orzel@amd.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed
Date: Fri, 15 Dec 2023 13:02:36 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2312151232580.3175268@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <add58ef45d9cb970c2447f22443f50c8@bugseng.com>

On Fri, 15 Dec 2023, Nicola Vetrini wrote:
> On 2023-12-14 23:32, Stefano Stabellini wrote:
> > On Thu, 14 Dec 2023, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 13/12/2023 14:02, Nicola Vetrini wrote:
> > > > On 2023-12-12 16:49, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 11/12/2023 12:32, Julien Grall wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > > > cannot return, but there is one path that may return, hence
> > > > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > > > I don't understand why this is necessary. The code should never be
> > > > > > reachable because do_unexpected_trap() is a noreturn().
> > > > >
> > > > > From the matrix discussion, it wasn't clear what was my position on
> > > this
> > > > > patch.
> > > > >
> > > > > I would much prefer if the breaks are kept. I could accept:
> > > > >
> > > > > ASSERT_UNREACHABLE();
> > > > > break;
> > > > >
> > > > > But this solution is a Nack because if you are concerned about
> > > functions
> > > > > like do_unexpected_trap() to return by mistaken, then it needs to also
> > > be
> > > > > safe in production.
> > > > >
> > > > > The current proposal is not safe.
> > 
> > I re-read the email thread. I also do not think that this is useful:
> > 
> >          do_unexpected_trap("SVE trap at EL2", regs);
> > -        break;
> > +        ASSERT_UNREACHABLE();
> > 
> > I also do not think that we should be concerned about functions like
> > do_unexpected_trap() to return by mistaken.
> > 
> > That said, what is the problem from MISRA point of view that we are
> > trying to fix? Is the only problem the presence of break; after the call
> > to a noreturn function?
> > 
> > If that's not allowed, I would suggest to do this:
> > 
> > 
> >          do_unexpected_trap("SVE trap at EL2", regs);
> > -        break;
> > +        /* break; */
> > 
> > 
> > Or deviate "break" globally as it doesn't seem to be a safety risk in my
> > opinion. If nothing else, it should make the code a bit safer because in
> > case of mistakes in do_unexpected_trap, at least we would continue to
> > follow a more reasonable code path rather than blindly falling through
> > the next switch case by accident.
> > 
> > 
> 
> That doesn't seem like a good idea to deviate just "break". However, Julien's
> earlier proposal
> 
> ASSERT_UNREACHABLE();
> break;
> 
> is ok, though it could be shrunk in a macro
> 
> #define unreachable_break ASSERT_UNREACHABLE(); break;
> 
> or just
> 
> #define unreachable_break break;
> 
> so that "unreachable_break" can be deviated.

Let's just go with:

ASSERT_UNREACHABLE();
break;

If Julien is OK with it too.

Just to make sure we are all aligned on what the problem and solution
are:
- we are not concerned that do_unexpected_trap could return
- it is just forbidden to have any code after something that doesn't
  return
- not even a break;
- so we need to use ASSERT_UNREACHABLE() as a marker to tell ECLAIR to
  ignore the violation