Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

CC MISRA C working group

Short summary: we are discussing whether the following is sufficient to
address MISRA C Rule 20.7, and also in general for safety:


 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )


as you can see "dn" has been parenthesizing because is used as a rhs,
while "pp" has *not* because it is used as lhs.

More below.


On Thu, 25 Aug 2022, Jan Beulich wrote:
> On 25.08.2022 10:02, Xenia Ragiadakou wrote:
> > On 8/22/22 14:48, Jan Beulich wrote:
> >> On 22.08.2022 12:43, Xenia Ragiadakou wrote:
> >>> On 8/22/22 12:59, Jan Beulich wrote:
> >>>> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
> >>>>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
> >>>>> dt_for_each_child_node(), add parentheses around the macro parameters 
> >>>>> that
> >>>>> have the arrow operator applied, to prevent against unintended 
> >>>>> expansions.
> >>>>
> >>>> Why is this relevant only when -> is used? For comparisons and the rhs of
> >>>> assignments it's as relevant, ad even for the lhs of assignments I doubt
> >>>> it can be generally omitted.
> >>>
> >>> Yes, I agree with you but some older patches that I sent that were
> >>> adding parentheses around the lhs of the assignments were not accepted
> >>> and I thought that the rhs of the assignments as well these comparisons
> >>> fall to the same category.
> >>>
> >>> Personally, I would expect to see parentheses, also, around the macro
> >>> parameters that are used as the lhs or the rhs of assignments, the
> >>> operands of comparison or the arguments of a function.
> >>> Not only because they can prevent against unintentional bugs but because
> >>> the parentheses help me to identify more easily the macro parameters
> >>> when reading a macro definition. I totally understand that for other
> >>> people parentheses may reduce readability.
> >>
> >> Afair Julien's comments were very specific to the lhs of assignments.
> >> So at the very least everything else ought to be parenthesized imo.
> >>
> > 
> > So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be 
> > for macro parameters used as the lhs of assignments and function arguments?
> 
> Afaic I don't consider that discussion settled.
> 
> > This feels a bit of a hack to parenthesize the macro parameters that are 
> > used as the rhs of an assignment but not those used as the lhs.
> 
> lhs and rhs of assignments are quite different, and hence making such a
> distinction wouldn't look to be a "hack" to me. In fact I've always
> considered this part of the language somewhat strange: To me
> parenthesizing e.g. an identifier already makes it (visually) an
> rvalue. Leaving aside odd (and easy to spot as odd) uses at the call
> sizes, I don't think I can come up with a case where parentheses are
> really needed. Anything needing parenthesizing actually yields an
> rvalue afaics, thus causing a diagnostic anyway.

Although I can see where you are coming from, parenthesizing an
identifier doesn't actually make it an rvalue. Also it is a lot simpler
to understand, review, and apply a policy that says:

"all macro parameters are parenthesized"

compared to a policy that says:

"most macro paremeters are parenthesized, let's go into the details of
which ones are parenthesized and which ones are not, including examples
and corner cases"

For simplicity, I would go with the simplest version, the MISRA version.

I am assuming that the MISRA Rule 20.7 requires that "pp" is also
parenthesized. Roberto, is that correct?


> >  From previous discussions on the topic, I understood that the 
> > parentheses are considered needed only when they eliminate operator 
> > precedence problems, while for the wrong parameter format bugs we can 
> > rely on the reviewers.
> > 
> > I think we need to decide if the rule will be applied as is and if not 
> > which will be the deviations along with their justification and add it 
> > to the document.
> 
> Yes. But this shouldn't hinder adjustments for all other, non-
> controversial cases.

It looks like we need a discussion and see where the majority of the
team is on this issue. I prefer the original MISRA version for
simplicity, but I also think it is OK if we make a small customization
to it. In that case, we would add the extra explanation and details to
docs/misra/rules.rst.

However, as we make the decision we also need to take into account that
if we don't go with the vanilla MISRA rule, there is a price to pay: all
the MISRA scanners, including cppcheck, Eclair, Coverity and others would
probably still flag these issues as violations polluting the results
and making the scanners less useful. We might have to mark each
"deviation" with a one-line in-code comment on top, or we would have to
disable automatic scanning for Rule 20.7 altogether. Either option is
not great.

This is actually the main reason why I prefer the vanillay MISRA
version: even if the resulting style might not be as good the the custom
version, we don't need to worry about reviewing this rule because we can
easily get automatic scans for it.


But first, let's confirm whether this change:


 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )


is sufficient to make the violation go away in Eclair or cppcheck.  I am
assuming it is not sufficient, but let's confirm.