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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 15:43, Nicola Vetrini wrote:
> On 16/08/2023 13:23, Jan Beulich wrote:
>> 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.)
> 
> The second statement is not quite correct, as some of the configurations 
> instruct the
> checker how to behave.

Well, I was referring to the one setting relevant here, and I added "aiui"
to indicate that I may be misreading what that file specifies.

Jan