[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.