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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?