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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)