Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 26 Jun 2024, Jan Beulich wrote:
> On 26.06.2024 03:11, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2024, Jan Beulich wrote:
> >> On 25.06.2024 02:54, Stefano Stabellini wrote:
> >>> On Mon, 24 Jun 2024, Federico Serafini wrote:
> >>>> Add break or pseudo keyword fallthrough to address violations of
> >>>> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
> >>>> every switch-clause".
> >>>>
> >>>> No functional change.
> >>>>
> >>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/x86/traps.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> >>>> index 9906e874d5..cbcec3fafb 100644
> >>>> --- a/xen/arch/x86/traps.c
> >>>> +++ b/xen/arch/x86/traps.c
> >>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, 
> >>>> uint32_t leaf,
> >>>>  
> >>>>      default:
> >>>>          ASSERT_UNREACHABLE();
> >>>> +        break;
> >>>
> >>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
> >>> statements" that can terminate a case, in addition to break.
> >>
> >> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
> >> Simply because of the rules needing to cover both debug and release builds.
> > 
> > The reason is that ASSERT_UNREACHABLE() might disappear from the release
> > build but it can still be used as a marker during static analysis. In
> > my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
> > which has an empty implementation in release builds.
> > 
> > The only reason I can think of to require a break; after an
> > ASSERT_UNREACHABLE() would be if we think the unreachability only apply
> > to debug build, not release build:
> > 
> > - debug build: it is unreachable
> > - release build: it is reachable
> > 
> > I don't think that is meant to be possible so I think we can use
> > ASSERT_UNREACHABLE() as a marker.
> 
> Well. For one such an assumption takes as a prereq that a debug build will
> be run through full coverage testing, i.e. all reachable paths proven to
> be taken. I understand that this prereq is intended to somehow be met,
> even if I'm having difficulty seeing how such a final proof would look
> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
> clearly any ASSERT_UNREACHABLE() must never be reached.
> 
> And then not covering for such cases takes the further assumption that
> debug and release builds are functionally identical. I'm afraid this would
> be a wrong assumption to make:
> 1) We may screw up somewhere, with code wrongly enabled only in one of the
>    two build modes.
> 2) The compiler may screw up, in particular with optimization.

I think there are two different issues here we are discussing.

One issue, like you said, has to do with coverage. It is important to
mark as "unreachable" any part of the code that is indeed unreachable
so that we can account it properly when we do coverage analysis. At the
moment the only "unreachable" marker that we have is
ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
coverage analysis we'll do.

However, there is a different separate question about what to do in the
Xen code after an ASSERT_UNREACHABLE(). E.g.:

             default:
                 ASSERT_UNREACHABLE();
                 return -EPERM; /* is it better with or without this? */
             }

Leaving coverage aside, would it be better to be defensive and actually
attempt to report errors back after an ASSERT_UNREACHABLE() like in the
example? Or is it better to assume the code is actually unreachable
hence there is no need to do anything afterwards?

One one hand, being defensive sounds good, on the other hand, any code
we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
which is also not good. In this example, there is no way to test the
return -EPERM code path. We also need to consider what is the right
thing to do if Xen finds itself in an erroneous situation such as being
in an unreachable code location.

So, after thinking about it and also talking to the safety manager, I
think we should:
- implement ASSERT_UNREACHABLE with a warning in release builds
- have "return -EPERM;" or similar for defensive programming