Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez
<martin.fernandez@eclypsium.com> wrote:
>
> On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez
> > <martin.fernandez@eclypsium.com> wrote:
> >>
> >> Add a description, an example and a possible workaround to the
> >> MACRO_ARG_REUSE check.
> >>
> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> >> ---
> >>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
> >>  1 file changed, 20 insertions(+)
> >>
> >> diff --git a/Documentation/dev-tools/checkpatch.rst
> >> b/Documentation/dev-tools/checkpatch.rst
> >> index b52452bc2963..86545c65cf7b 100644
> >> --- a/Documentation/dev-tools/checkpatch.rst
> >> +++ b/Documentation/dev-tools/checkpatch.rst
> >> @@ -759,6 +759,26 @@ Indentation and Line Breaks
> >>  Macros, Attributes and Symbols
> >>  ------------------------------
> >>
> >> +  **ARG_REUSE**
> >
> > The name of this checkpatch type is actually "MACRO_ARG_REUSE".
>
> You are right.
>
> >> +    Using the same argument multiple times in the macro definition
> >> +    would lead to unwanted side-effects.
> >
> > how about "... may lead to unwanted side effects"?
> >
> > Rationale: it does only lead to side effects if there are multiple
> > computations involved.
>
> Good point.
>
> > just on spelling:
> > s/side-effects/side effects/
> >
> >> +
> >> +    For example, given a `min` macro defined like::
> >> +
> >> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
> >> +
> >> +    If you call it with `min(foo(x), 0)`, it would expand to::
> >> +
> >> +      foo(x) < 0 ? foo(x) : 0
> >> +
> >> +    If `foo` has side-effects or it's an expensive calculation the
> >> +    results might not be what the user intended.
> >> +
> >
> > s/side-effects/side effects/
> >
> >> +    For a workaround the idea is to define local variables to hold the
> >> +    macro's arguments. Checkout the actual implementation of `min` in
> >> +    include/linux/minmax.h for the full implementation of the
> >> +    workaround.
> >> +
> >
> > I ran checkpatch on all commits from v5.17..v5.18 and looked for all
> > check warnings with MACRO_ARG_REUSE.
> >
> > There were 35 warnings in 15 commits, touching 16 different files (4
> > in drivers/staging, 5 in drivers/net/wireless/, 5 in
> > drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/).
> >
> > As far as I see it from those commits, the more common way to address
> > this is to check that a macro is only used locally in some file and
> > that all uses of that macro pass a constant value as macro argument.
> >
> > Maybe we add these two as equally good alternatives?
>
> Yes, that's what I did on my patch that triggered this patch. But I
> don't think that's a workaround. You still have the issue there, just
> that the uses of the macros are "good".
>
> I think that falls better into the "I know what I'm doing, I'm ok with
> the warning" scenario, than a proper workaround.
>

Well, the purpose of the checkpatch documentation is to provide some
more background information on the warning (e.g., the historic
motivation, what to consider when judging its validity) and any hints
on possible resolutions. So, I would expect to see the documentation
cover explaining the most common (reusable) resolutions. A valid
argument why a check warning can be ignored falls into such a
resolution. In fact, the category "CHECK" in checkpatch.pl already
suggests that often the resolution may be to "inspect some code, but
not modify the code and then further 'ignore' the reported warning",
as some rules in checkpatch are checking something with just some
quite weak heuristics.

So, for this patch here: How about avoiding the word "workaround" and
just state these to options as resolution, e.g., a text like this:

Here are two possible options:
- Check the macro arguments of all uses of this macro to be free of
unintended side effects. Passing a constant value is usually fine, as
the compiler will use constant propagation and further optimizations
to produce acceptable code.
- If needed, define local variables in the macro to hold the macro's
argument. See the implementation of `min` in include/linux/minmax.h as
one example of this option.

What do you think?

I really appreciate you providing some documentation for this rule. I
also appreciate the rules that checkpatch.pl checks being better
explained to all of us in the kernel community. That avoids that we
all, especially newcomers, individually wonder about what checkpatch
intends to warn us about.

Lukas