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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 16 Aug 2023, 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.)

Jan has a point: I think we should record all our deviations and unique
ways to interpret the rules under docs/misra. And the Eclair
configuration should reflect that. It is not a good idea to only keep
the information in the Eclair config because, even if it is now upstream
in xen.git, it is still a tool-specific configuration file -- it is not
a proper document. MISRA C and its interpretation is important enough
that we should have a plain English document to cover it (which now is
docs/misra/rules.rst).

Nicola, I volunteer to send patches to make any necessary updates to
docs/misra/rules.rst and other docs. Please send out to me a list of
deviations/configurations and I'll make sure to reflect them under
docs/misra so that they are in sync.

Cheers,

Stefano