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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 12/12/2023 01:36, Stefano Stabellini wrote:
> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> > On 2023-12-11 17:05, Julien Grall wrote:
> > > On 11/12/2023 15:59, Julien Grall wrote:
> > > > Hi Nicola,
> > > >
> > > > On 11/12/2023 14:54, Nicola Vetrini wrote:
> > > > > On 2023-12-11 13: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().
> > > > > >
> > > > > > Cheers,
> > > > > It was meant as a safeguard against mistakes.
> > > > I am confused about which mistakes you are trying to prevent. Are you
> > > > saying you are not trusting the noreturn attribute?
> > > >
> > > > And if so, are you intending to add ASSERT_UNREACHABLE() after every
> > > > single call to noreturn functions?
> > > Replying to myself. What's confusing the most is that in [1], you decided to
> > > not add the ASSERT_UNREACHABLE(). Yet the problem is similar.
> > >
> > > I'd also like to point out that by removing the "break", then if the
> > > 'noreturn' function turns out to return, then in prod build you would
> > > fallthrough to the next case. And who knows what's going to happen...
> > >
> > > All of this really adds some confusion...
> > I should have checked before responding: do_trap_hyp_sync is not afaik
> > noreturn. Specifically, do_trap_brk may return. If I worked under the wrong
> > assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped.
> It looks like we could add noreturn to do_trap_brk. Julien what do you
> think?
You can't add noreturn to do_trap_brk(). It is used to handle WARN_ON() 
that *will* return.
Cheers,

--
Julien Grall