Re: [XEN PATCH v2 1/3] EFI: address a violation of MISRA C Rule 13.6

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 08, 2024 at 02:49:52PM +0200, Roberto Bagnara wrote:
> On 2024-10-08 07:59, Jan Beulich wrote:
> > On 02.10.2024 08:54, Roberto Bagnara wrote:
> > > On 2024-10-02 08:09, Jan Beulich wrote:
> > > > On 01.10.2024 23:36, Stefano Stabellini wrote:
> > > > > On Tue, 1 Oct 2024, Jan Beulich wrote:
> > > > > > On 01.10.2024 07:25, Roberto Bagnara wrote:
> > > > > > > On 2024-09-30 15:07, Jan Beulich wrote:
> > > > > > > > On 30.09.2024 14:49, Federico Serafini wrote:
> > > > > > > > > guest_handle_ok()'s expansion contains a sizeof() involving 
> > > > > > > > > its
> > > > > > > > > first argument which is guest_handle_cast().
> > > > > > > > > The expansion of the latter, in turn, contains a variable
> > > > > > > > > initialization.
> > > > > > > > > 
> > > > > > > > > Since MISRA considers the initialization (even of a local 
> > > > > > > > > variable)
> > > > > > > > > a side effect, the chain of expansions mentioned above 
> > > > > > > > > violates
> > > > > > > > > MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator 
> > > > > > > > > shall not
> > > > > > > > > contain any expression which has potential side effect).
> > > > > > > > 
> > > > > > > > I'm afraid I need to ask for clarification of terminology and 
> > > > > > > > alike here.
> > > > > > > > While the Misra doc has a section on Persistent Side Effects in 
> > > > > > > > its
> > > > > > > > Glossary appendix, what constitutes a side effect from its pov 
> > > > > > > > isn't
> > > > > > > > really spelled out anywhere. Which in turn raises the question 
> > > > > > > > whether it
> > > > > > > > is indeed Misra (and not just Eclair) which deems 
> > > > > > > > initialization a side
> > > > > > > > effect. This is even more so relevant as 13.6 talks of only 
> > > > > > > > expressions,
> > > > > > > > yet initializers fall under declarations (in turn involving an 
> > > > > > > > expression
> > > > > > > > on the rhs of the equal sign).
> > > > > > > > 
> > > > > > > > All the same of course affects patch 2 then, too.
> > > > > > > 
> > > > > > > MISRA C leaves the definition of "side effect" to the C Standard.
> > > > > > > E.g., C18 5.1.2.3p2:
> > > > > > > 
> > > > > > >      Accessing a volatile object, modifying an object, modifying 
> > > > > > > a file,
> > > > > > >      or calling a function that does any of those operations are 
> > > > > > > all
> > > > > > >      side effects,[omitted irrelevant footnote reference] which 
> > > > > > > are
> > > > > > >      changes in the state of the execution environment.
> > > > > > > 
> > > > > > > The MISRA C:2012/2023 Glossary entry for "Persistent side effect"
> > > > > > > indirectly confirms that initialization is always a side effect.
> > > > > > 
> > > > > > Hmm, that's interesting: There's indeed an example with an 
> > > > > > initializer
> > > > > > there. Yet to me the text you quote from the C standard does not say
> > > > > > that initialization is a side effect - it would be "modifying an
> > > > > > object" aiui, yet ahead of initialization being complete the object
> > > > > > doesn't "exist" imo, and hence can be "modified" only afterwards.
> > > > > 
> > > > > I feel it's becoming a bit too philosophical. Since there's some room
> > > > > for interpretation and only two violations left to address, I believe
> > > > > it's best to stick with the stricter interpretation of the definition.
> > > > > Therefore, I'd proceed with this series in its current form.
> > > > 
> > > > Proceeding with the series in its current form may be okay (as you say,
> > > > you view the changes as readability improvements anyway), but imo the
> > > > interpretation needs settling on no matter what. In fact even for these
> > > > two patches it may affect what their descriptions ought to say (would
> > > > be nice imo to avoid permanently recording potentially misleading
> > > > information by committing as is). And of course clarity would help
> > > > dealing with future instances that might appear. I take it you realize
> > > > that if someone had submitted a patch adding code similar to the
> > > > original forms of what's being altered here, it would be relatively
> > > > unlikely for a reviewer to spot the issue. IOW here we're making
> > > > ourselves heavily dependent upon Eclair spotting (supposed) issues,
> > > > adding extra work and delays for such changes to go in.
> > > 
> > > You can do two things to obtain a second opinion:
> > > 
> > > 1) Use the MISRA forum (here is the link to the forum
> > >      section devoted to the side-effect rules of MISRA C:2012
> > >      and MISRA C:2023 
> > > (https://forum.misra.org.uk/forumdisplay.php?fid=168).
> > >      The MISRA C Working Group will, in due course, provide
> > >      you with an official answer to your questions about what,
> > >      for the interpretation of Rule 13.6, has to be considered
> > >      a side effect.
> > > 
> > > 2) Reach out to your ISO National Body and try to obtain
> > >      an official answer from ISO/IEC JTC1/SC22/WG14 (the
> > >      international standardization working group for the
> > >      programming language C) to your questions about what the
> > >      C Standard considers to be side effects.
> > 
> > I took the latter route, and to my (positive) surprise I got back an answer
> > the same day. There was a request for someone to confirm, but so far I 
> > didn't
> > see further replies. Since this is a German institution I raised the 
> > question
> > in German and got back an answer in German (attached); I've tried my best to
> > translate this without falsifying anything, but I've omitted non-technical
> > parts:
> > 
> > "Initialization of an object is never a side effect of the initialization
> > by itself. Of course expressions used for initialization can themselves have
> > side effects on other objects.
> > 
> > @Andreas: Do you agree? C after all has a far simpler object model than C++.
> > The (potential) change in object representation (i.e. the bytes underlying
> > the object) is not a side effect of object initialization, but its primary
> > purpose."
> > 
> > Further for Misra she added a reference to a Swiss person, but I think with
> > Bugseng we have sufficient expertise there.
> 
> Unfortunately, the (translation of the) answer you received adds
> confusion to previous confusion: who answered has highlighted the
> "side" part of the term, which is indeed relevant in computer science,
> but not for the C standard.  To the point that the same argument could
> be used to deny that ++i has a side effect because the increment is
> the "primary" effect...
> 
> Part of the confusion is maybe in the the following paragraph Jan
> wrote earlier:
> 
> > Hmm, that's interesting: There's indeed an example with an initializer
> > there. Yet to me the text you quote from the C standard does not say
> > that initialization is a side effect - it would be "modifying an
> > object" aiui, yet ahead of initialization being complete the object
> > doesn't "exist" imo, and hence can be "modified" only afterwards.
> 
> In C, it is not true that the object does not exist ahead of
> initialization.  Try the following:
> 
> extern int f(int* p);
> 
> int main() {
>   int i = f(&i);
> }

This is interesting discussion, but I don't think it needs to block
anything. The proposed change doesn't violate any other rule/code style,
and I'd argue it's more readable. Taking more strict interpretation in
this case doesn't really hurt. I already acked this patch.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature