Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 12:01, Nicola Vetrini wrote:
> Hi,
> 
> On 08/08/2023 11:03, Nicola Vetrini wrote:
>> On 04/08/2023 08:42, Jan Beulich wrote:
>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>>>>
>>>>>> The functions
>>>>>> - machine_halt
>>>>>> - maybe_reboot
>>>>>> - machine_restart
>>>>>> are not supposed to return, hence the following break statement
>>>>>> is marked as intentionally unreachable with the 
>>>>>> ASSERT_UNREACHABLE()
>>>>>> macro to justify the violation of the rule.
>>>>>
>>>>> During the discussion it was mentioned that this won't help with
>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>> effectively nothing. You want to clarify here how release builds
>>>>> are to be taken care of, as those are what eventual certification
>>>>> will be run against.
>>>>
>>>> Something along these lines:
>>>>
>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to 
>>>> actually
>>>> assert and detect errors, but it is also used as a marker to tag
>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't 
>>>> resolve
>>>> into an assert, but retains its role of a code marker.
>>>>
>>>> Does it work?
>>>
>>> Well, it states what is happening, but I'm not convinced it satisfies
>>> rule 2.1. There's then still code there which isn't reachable, and
>>> which a scanner will spot and report.
>>
>> It's not clear to me whether you dislike the patch itself or the commit
>> message. If it's the latter, how about:
>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>> unreachable code, which
>> constitutes a motivated deviation from Rule 2.1. Additionally, in 
>> non-release
>> builds, this macro performs a failing assertion to detect errors."
> 
> Any feedback on this (with one edit: s/a failing assertion/an 
> assertion/)

The patch here is kind of okay, but I'm afraid I view my earlier question
as not addressed: How will the proposed change prevent the scanner from
spotting issues here in release builds? Just stating in the description
that there's a deviation is not going to help that.

Jan