[v4,1/1] arch/sh: avoid spurious sizeof-pointer-div warning

Commit Message

Michael Karcher Jan. 23, 2023, 6:48 p.m. UTC

Gcc warns about the pattern sizeof(void*)/sizeof(void), as it looks like
the abuse of a pattern to calculate the array size. This pattern appears
in the unevaluated part of the ternary operator in _INTC_ARRAY if the
parameter is NULL.

The replacement uses an alternate approach to return 0 in case of NULL
which does not generate the pattern sizeof(void*)/sizeof(void), but still
emits the warning if _INTC_ARRAY is called with a nonarray parameter.

This patch is required for successful compilation with -Werror enabled.

The idea to use _Generic for type distinction is taken from Comment #7
in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108483 by Jakub Jelinek

Signed-off-by: Michael Karcher <[e-mail]>
---
This edition of the mail has the correct "v4" designation in the subject.

History:
v4:
   - Put the case distinction into the numerator instead of the denominator
   - Refactor the case disctinction into a second macro
v3:
   - I had a stern discussion with Thunderbird about not mangling the
     space characters in my email, and I hope spaces get sent as standard
     spaces now
v2:
   - improve title and remove mostly redundant first sentence of the
     description
   - adjust formatting of the _Generic construction

Comments

Randy Dunlap Jan. 23, 2023, 7:37 p.m. UTC | #1

Hi,

I'm curious how you generated this patch.
'patch' complains:

patch: **** malformed patch at line 53:                prio_regs,    sense_regs, ack_regs)    \

(more below)

On 1/23/23 10:48, Michael Karcher wrote:
> Gcc warns about the pattern sizeof(void*)/sizeof(void), as it looks like
> the abuse of a pattern to calculate the array size. This pattern appears
> in the unevaluated part of the ternary operator in _INTC_ARRAY if the
> parameter is NULL.
> 
> The replacement uses an alternate approach to return 0 in case of NULL
> which does not generate the pattern sizeof(void*)/sizeof(void), but still
> emits the warning if _INTC_ARRAY is called with a nonarray parameter.
> 
> This patch is required for successful compilation with -Werror enabled.
> 
> The idea to use _Generic for type distinction is taken from Comment #7
> in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108483 by Jakub Jelinek
> 
> Signed-off-by: Michael Karcher <[e-mail]>
> ---
> This edition of the mail has the correct "v4" designation in the subject.
> 
> History:
> v4:
>   - Put the case distinction into the numerator instead of the denominator
>   - Refactor the case disctinction into a second macro
> v3:
>   - I had a stern discussion with Thunderbird about not mangling the
>     space characters in my email, and I hope spaces get sent as standard
>     spaces now
> v2:
>   - improve title and remove mostly redundant first sentence of the
>     description
>   - adjust formatting of the _Generic construction
> 
> diff --git a/include/linux/sh_intc.h b/include/linux/sh_intc.h
> index c255273b0281..98d1da0d8e36 100644
> --- a/include/linux/sh_intc.h
> +++ b/include/linux/sh_intc.h
> @@ -97,7 +97,9 @@ struct intc_hw_desc {

That ,9 should be ,10.

With that change:

Acked-by: Randy Dunlap <[e-mail]> # build-tested

Thanks.

>      unsigned int nr_subgroups;
>  };
> 
> -#define _INTC_ARRAY(a) a, __same_type(a, NULL) ? 0 : sizeof(a)/sizeof(*a)
> +#define _INTC_SIZEOF_OR_ZERO(a) (_Generic(a,                 \
> +                                 typeof(NULL):  0,           \
> +                                 default:       sizeof(a)))
> +#define _INTC_ARRAY(a) a, _INTC_SIZEOF_OR_ZERO(a)/sizeof(*a)
> 
>  #define INTC_HW_DESC(vectors, groups, mask_regs,    \
>               prio_regs,    sense_regs, ack_regs)    \
> 
>

Geert Uytterhoeven Jan. 24, 2023, 1:07 p.m. UTC | #2

On Mon, Jan 23, 2023 at 8:40 PM Randy Dunlap <[e-mail]> wrote:
> I'm curious how you generated this patch.
> 'patch' complains:
>
> patch: **** malformed patch at line 53:                prio_regs,    sense_regs, ack_regs)    \
>
> (more below)
>
> On 1/23/23 10:48, Michael Karcher wrote:
> > Gcc warns about the pattern sizeof(void*)/sizeof(void), as it looks like
> > the abuse of a pattern to calculate the array size. This pattern appears
> > in the unevaluated part of the ternary operator in _INTC_ARRAY if the
> > parameter is NULL.
> >
> > The replacement uses an alternate approach to return 0 in case of NULL
> > which does not generate the pattern sizeof(void*)/sizeof(void), but still
> > emits the warning if _INTC_ARRAY is called with a nonarray parameter.
> >
> > This patch is required for successful compilation with -Werror enabled.
> >
> > The idea to use _Generic for type distinction is taken from Comment #7
> > in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108483 by Jakub Jelinek
> >
> > Signed-off-by: Michael Karcher <[e-mail]>
> > ---
> > This edition of the mail has the correct "v4" designation in the subject.
> >
> > History:
> > v4:
> >   - Put the case distinction into the numerator instead of the denominator
> >   - Refactor the case disctinction into a second macro
> > v3:
> >   - I had a stern discussion with Thunderbird about not mangling the
> >     space characters in my email, and I hope spaces get sent as standard
> >     spaces now
> > v2:
> >   - improve title and remove mostly redundant first sentence of the
> >     description
> >   - adjust formatting of the _Generic construction
> >
> > diff --git a/include/linux/sh_intc.h b/include/linux/sh_intc.h
> > index c255273b0281..98d1da0d8e36 100644
> > --- a/include/linux/sh_intc.h
> > +++ b/include/linux/sh_intc.h
> > @@ -97,7 +97,9 @@ struct intc_hw_desc {
>
> That ,9 should be ,10.

Indeed.

Reviewed-by: Geert Uytterhoeven <[e-mail]>
Tested-by: Geert Uytterhoeven <[e-mail]>

Note that I didn't receive v5 (neither did lore), only Adrian's reply.

> > -#define _INTC_ARRAY(a) a, __same_type(a, NULL) ? 0 : sizeof(a)/sizeof(*a)
> > +#define _INTC_SIZEOF_OR_ZERO(a) (_Generic(a,                 \
> > +                                 typeof(NULL):  0,           \
> > +                                 default:       sizeof(a)))

_INTC_SIZEOF_OR_ZERO() might be useful in general.

> > +#define _INTC_ARRAY(a) a, _INTC_SIZEOF_OR_ZERO(a)/sizeof(*a)
> >
> >  #define INTC_HW_DESC(vectors, groups, mask_regs,    \
> >               prio_regs,    sense_regs, ack_regs)    \

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Michael Karcher Jan. 24, 2023, 10:05 p.m. UTC | #3

Am 24.01.2023 um 14:07 schrieb Geert Uytterhoeven:

> On Mon, Jan 23, 2023 at 8:40 PM Randy Dunlap <[e-mail]> wrote:
>> I'm curious how you generated this patch.

I'm currently using Thunderbird in a GUI environment as my only mail client,
and I pasted just the +/- lines of the patch into the pre-composed mail. The
copious amounts of bad mails I generated this way are a good point to set up
git-send-email in my linux environment to avoid bad patches / badly formatted
mails

> Reviewed-by: Geert Uytterhoeven <[e-mail]>
> Tested-by: Geert Uytterhoeven <[e-mail]>

Thanks!

> Note that I didn't receive v5 (neither did lore), only Adrian's reply.

And that is another issue of my mail setup. Thunderbird is set to
automatically decide on HTML or plaintext only, and as soon as I switch to
"preformatted text", Thunderbird concludes that the mail contains
significant formatting, and defaults to multipart/alternative with HTML and
plain text. Mails of this type obviously get dropped by vger.kernel.org.

On the other hand, if I *don't* select "preformatted text", but use the
default settings "paragraph" or "normal text", there will be no HTML part
(which is good), but spaces will be transformed to non-breaking spaces.

I verified that the version of the v5 mail I just sent to all recipients
I already had on the v1 mail (so that doesn't include Geert, but both lkml
and linux-sh) now fulfills all requirements. I verified that it appeared
at lore on linux-sh.

>>> -#define _INTC_ARRAY(a) a, __same_type(a, NULL) ? 0 : sizeof(a)/sizeof(*a)
>>> +#define _INTC_SIZEOF_OR_ZERO(a) (_Generic(a,                 \
>>> +                                 typeof(NULL):  0,           \
>>> +                                 default:       sizeof(a)))
> _INTC_SIZEOF_OR_ZERO() might be useful in general.

Indeed. Feel free to move it (using a name that doesn't start with _INTC) to
a more general linux header file.

On the other hand, having a sizeof-like macro that fails to work on void*
(what NULL is), but works on all other types might be confusing and
error-prone. You likely want to further type constrain it, and you need to
parenthesize the argument "a" if you generalize this macro.

The version suggested by Jakub Jelinek in the gcc bugtracker has the advantage
of verifying that the void* parameter is indeed the compiletime constant NULL,
but it has the disadvantage that the expression used to verify that NULL was
passed is involving pointers (it needs to, as NULL is a pointer), and an
expression involving pointers is not an integral constant expression according
to C90 rules, so the compile-time assert using a possibly negatively sized
array is rejected by -Wvla, which is enabled in the kernel source code by
default.

Kind regards,
   Michael Karcher

Randy Dunlap Jan. 26, 2023, 6:18 p.m. UTC | #4

Hi Michael,

On 1/24/23 14:05, Michael Karcher wrote:
> Am 24.01.2023 um 14:07 schrieb Geert Uytterhoeven:
> 
>> On Mon, Jan 23, 2023 at 8:40 PM Randy Dunlap <[e-mail]> wrote:
>>> I'm curious how you generated this patch.
> 
> I'm currently using Thunderbird in a GUI environment as my only mail client,
> and I pasted just the +/- lines of the patch into the pre-composed mail. The
> copious amounts of bad mails I generated this way are a good point to set up
> git-send-email in my linux environment to avoid bad patches / badly formatted
> mails
> 

If there is anything that you told Thunderbird about as far as sending plain text
patches that is not already documented[1], please consider adding it to the
documentation.

[1] https://docs.kernel.org/process/email-clients.html (+ search for Thunderbird)
or linux/Documentation/process/email-clients.rst

Thanks.

Patch

diff --git a/include/linux/sh_intc.h b/include/linux/sh_intc.h
index c255273b0281..98d1da0d8e36 100644
--- a/include/linux/sh_intc.h
+++ b/include/linux/sh_intc.h
@@ -97,7 +97,9 @@  struct intc_hw_desc {
      unsigned int nr_subgroups;
  };

-#define _INTC_ARRAY(a) a, __same_type(a, NULL) ? 0 : sizeof(a)/sizeof(*a)
+#define _INTC_SIZEOF_OR_ZERO(a) (_Generic(a,                 \
+                                 typeof(NULL):  0,           \
+                                 default:       sizeof(a)))
+#define _INTC_ARRAY(a) a, _INTC_SIZEOF_OR_ZERO(a)/sizeof(*a)

  #define INTC_HW_DESC(vectors, groups, mask_regs,    \
               prio_regs,    sense_regs, ack_regs)    \