[PATCH] kbuild: Disable -Wpointer-to-enum-cast

[Nathan Chancellor]

Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
casting to enums. The kernel does this in certain places, such as device
tree matches to set the version of the device being used, which allows
the kernel to avoid using a gigantic union.

https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264

To avoid a ton of false positive warnings, disable this particular part
of the warning, which has been split off into a separate diagnostic so
that the entire warning does not need to be turned off for clang.

Cc: sta...@vger.kernel.org
Link: https://github.com/ClangBuiltLinux/linux/issues/887
Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
Signed-off-by: Nathan Chancellor <natecha...@gmail.com>
---
Makefile | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 86035d866f2c..90e56d5657c9 100644
--- a/Makefile
+++ b/Makefile
@@ -748,6 +748,10 @@ KBUILD_CFLAGS += -Wno-tautological-compare
# source of a reference will be _MergedGlobals and not on of the whitelisted names.
# See modpost pattern 2
KBUILD_CFLAGS += -mno-global-merge
+# clang's -Wpointer-to-int-cast warns when casting to enums, which does not match GCC.
+# Disable that part of the warning because it is very noisy across the kernel and does
+# not point out any real bugs.
+KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
else

# These warnings generated too much noise in a regular build.
--
2.25.1

[Masahiro Yamada]

Hi Nathan,

I'd rather want to fix all the call-sites (97 drivers?)
instead of having -Wno-pointer-to-enum-cast forever.

If it is tedious to fix them all for now, can we add it
into scripts/Makefile.extrawarn so that this is disabled
by default, but shows up with W=1 builds?

(When we fix most of them, we will be able to
make it a real warning.)


What do you think?

Thanks.

> # These warnings generated too much noise in a regular build.
> --
> 2.25.1
>

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200308073400.23398-1-natechancellor%40gmail.com.



--
Best Regards
Masahiro Yamada

[Nathan Chancellor]

Yes, there are 97 unique warnings across my builds, which are mainly
arm, arm64, and x86_64 defconfig/allmodconfig/allyesconfig:

https://github.com/ClangBuiltLinux/linux/issues/887#issuecomment-587938406

> If it is tedious to fix them all for now, can we add it
> into scripts/Makefile.extrawarn so that this is disabled
> by default, but shows up with W=1 builds?

Sure, I can send v2 to do that but I think that sending 97 patches just
casting the small values (usually less than twenty) to unsigned long
then to the enum is rather frivolous. I audited at least ten to fifteen
of these call sites when creating the clang patch and they are all
basically false positives.

I believe Nick discussed this with some other developers off list, maybe
he has some other feedback to give. I'll wait to send a v2 until
tomorrow in case anyone else has further comments.

> (When we fix most of them, we will be able to
> make it a real warning.)
>
>
> What do you think?
>
> Thanks.

Cheers,
Nathan

[David Laight]

From: Nathan Chancellor
> Sent: 10 March 2020 01:26
...

> Sure, I can send v2 to do that but I think that sending 97 patches just
> casting the small values (usually less than twenty) to unsigned long
> then to the enum is rather frivolous. I audited at least ten to fifteen
> of these call sites when creating the clang patch and they are all
> basically false positives.

Such casts just make the code hard to read.
If misused casts can hide horrid bugs.
IMHO sprinkling the code with casts just to remove
compiler warnings will bite back one day.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[Masahiro Yamada]

On Tue, Mar 10, 2020 at 8:31 PM David Laight <David....@aculab.com> wrote:
>
> From: Nathan Chancellor
> > Sent: 10 March 2020 01:26
> ...
> > Sure, I can send v2 to do that but I think that sending 97 patches just
> > casting the small values (usually less than twenty) to unsigned long
> > then to the enum is rather frivolous. I audited at least ten to fifteen
> > of these call sites when creating the clang patch and they are all
> > basically false positives.
>
> Such casts just make the code hard to read.
> If misused casts can hide horrid bugs.
> IMHO sprinkling the code with casts just to remove
> compiler warnings will bite back one day.
>

I agree that too much casts make the code hard to read,
but irrespective of this patch, there is no difference
in the fact that we need a cast to convert
(const void *) to a non-pointer value.

The difference is whether we use
(uintptr_t) or (enum foo).




If we want to avoid casts completely,
we could use union in struct of_device_id
although this might be rejected.


FWIW:

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 6853dbb4131d..534170bea134 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -415,11 +415,11 @@ static struct scsi_host_template ahci_platform_sht = {
};

static const struct of_device_id ahci_of_match[] = {
- {.compatible = "brcm,bcm7425-ahci", .data = (void *)BRCM_SATA_BCM7425},
- {.compatible = "brcm,bcm7445-ahci", .data = (void *)BRCM_SATA_BCM7445},
- {.compatible = "brcm,bcm63138-ahci", .data = (void *)BRCM_SATA_BCM7445},
- {.compatible = "brcm,bcm-nsp-ahci", .data = (void *)BRCM_SATA_NSP},
- {.compatible = "brcm,bcm7216-ahci", .data = (void *)BRCM_SATA_BCM7216},
+ {.compatible = "brcm,bcm7425-ahci", .data2 = BRCM_SATA_BCM7425},
+ {.compatible = "brcm,bcm7445-ahci", .data2 = BRCM_SATA_BCM7445},
+ {.compatible = "brcm,bcm63138-ahci", .data2 = BRCM_SATA_BCM7445},
+ {.compatible = "brcm,bcm-nsp-ahci", .data2 = BRCM_SATA_NSP},
+ {.compatible = "brcm,bcm7216-ahci", .data2 = BRCM_SATA_BCM7216},
{},
};
MODULE_DEVICE_TABLE(of, ahci_of_match);
@@ -442,7 +442,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (!of_id)
return -ENODEV;

- priv->version = (enum brcm_ahci_version)of_id->data;
+ priv->version = of_id->data2;
priv->dev = dev;

res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "top-ctrl");
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index e3596db077dc..98d44ebf146a 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -261,7 +261,10 @@ struct of_device_id {
char name[32];
char type[32];
char compatible[128];
- const void *data;
+ union {
+ const void *data;
+ unsigned long data2;
+ };
};

/* VIO */

[Nick Desaulniers]

+ Saravana, who I spoke to briefly about this.

--
Thanks,
~Nick Desaulniers

[Joe Perches]

On Wed, 2020-03-11 at 00:30 +0900, Masahiro Yamada wrote:
> On Tue, Mar 10, 2020 at 8:31 PM David Laight <David....@aculab.com> wrote:
> > From: Nathan Chancellor
> > > Sent: 10 March 2020 01:26
> > ...
> > > Sure, I can send v2 to do that but I think that sending 97 patches just
> > > casting the small values (usually less than twenty) to unsigned long
> > > then to the enum is rather frivolous. I audited at least ten to fifteen
> > > of these call sites when creating the clang patch and they are all
> > > basically false positives.
> >
> > Such casts just make the code hard to read.
> > If misused casts can hide horrid bugs.
> > IMHO sprinkling the code with casts just to remove
> > compiler warnings will bite back one day.
> >
>
> I agree that too much casts make the code hard to read,
> but irrespective of this patch, there is no difference
> in the fact that we need a cast to convert
> (const void *) to a non-pointer value.
>
> The difference is whether we use
> (uintptr_t) or (enum foo).

or hide it altogether in a macro like cast_to

#define cast_to(type, val) \
etc...

where cast_to could do the appropriate
intermediate cast if type is an enum
and sizeof(tupeof val) is larger than int

[Saravana Kannan]

The union like you suggested might fly. Maybe the new field data_ulong
or data_u32 might work and even help non-enum non-pointer values to be
stored in this directly too without needing the casting that's needed
today.

I still don't get why the compiler can't be smarter about this. If the
enum would fit inside the pointer, why not leave that alone and throw
a warning only when the enum really can overflow the pointer field?

I've never (or long since forgotten) consciously declared a union
without a name and directly accessed it's fields. If this compiles,
this seems reasonable.

-Saravana

[Nathan Chancellor]

Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
casting to enums. The kernel does this in certain places, such as device
tree matches to set the version of the device being used, which allows
the kernel to avoid using a gigantic union.

https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264

To avoid a ton of false positive warnings, disable this particular part
of the warning, which has been split off into a separate diagnostic so

that the entire warning does not need to be turned off for clang. It
will be visible under W=1 in case people want to go about fixing these
easily and enabling the warning treewide.

Cc: sta...@vger.kernel.org
Link: https://github.com/ClangBuiltLinux/linux/issues/887
Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
Signed-off-by: Nathan Chancellor <natecha...@gmail.com>
---

v1 -> v2:

* Move under scripts/Makefile.extrawarn, as requested by Masahiro

scripts/Makefile.extrawarn | 1 +
1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index ecddf83ac142..ca08f2fe7c34 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -48,6 +48,7 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
KBUILD_CFLAGS += -Wno-format
KBUILD_CFLAGS += -Wno-sign-compare
KBUILD_CFLAGS += -Wno-format-zero-length

+KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)

endif

endif
--
2.26.0.rc1

[Masahiro Yamada]

On Thu, Mar 12, 2020 at 4:41 AM Nathan Chancellor
<natecha...@gmail.com> wrote:
>
> Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
> casting to enums. The kernel does this in certain places, such as device
> tree matches to set the version of the device being used, which allows
> the kernel to avoid using a gigantic union.
>
> https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
> https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
> https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264
>
> To avoid a ton of false positive warnings, disable this particular part
> of the warning, which has been split off into a separate diagnostic so
> that the entire warning does not need to be turned off for clang. It
> will be visible under W=1 in case people want to go about fixing these
> easily and enabling the warning treewide.
>
> Cc: sta...@vger.kernel.org
> Link: https://github.com/ClangBuiltLinux/linux/issues/887
> Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
> Signed-off-by: Nathan Chancellor <natecha...@gmail.com>
> ---

Applied to linux-kbuild.
Thanks.

>
> v1 -> v2:
>
> * Move under scripts/Makefile.extrawarn, as requested by Masahiro
>
> scripts/Makefile.extrawarn | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index ecddf83ac142..ca08f2fe7c34 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -48,6 +48,7 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> KBUILD_CFLAGS += -Wno-format
> KBUILD_CFLAGS += -Wno-sign-compare
> KBUILD_CFLAGS += -Wno-format-zero-length
> +KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
> endif
>
> endif
> --
> 2.26.0.rc1
>

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200311194121.38047-1-natechancellor%40gmail.com.