[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v2 1/3] EFI: address a violation of MISRA C Rule 13.6
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |