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

From: Julien Grall <julien@xen.org>
To: Stefano Stabellini <sstabellini@kernel.org>,
	Nicola Vetrini <nicola.vetrini@bugseng.com>
Cc: 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: Tue, 12 Dec 2023 09:23:51 +0000	[thread overview]
Message-ID: <a9489f10-b126-40fa-9b1e-6a8a47dcc071@xen.org> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2312111735500.1703076@ubuntu-linux-20-04-desktop>

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