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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 12:47, Nicola Vetrini wrote:
> On 16/08/2023 12:31, Jan Beulich wrote:
>> On 16.08.2023 12:01, Nicola Vetrini wrote:
>>> 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.
> 
> There is a deviation already in place. At the moment it just ignores 
> anything below an unreachable ASSERT_UNREACHABLE(), no matter what that 
> macro will expand to.

What do you mean by "in place"? docs/misra/ has nothing, afaics. And
automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering
things out of reports, aiui. (Plus of course there should be a single
central place where all deviations are recorded, imo.)

Jan