Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
>
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
>
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
So looks like the bulk of these are:
switch (x) {
case 0:
++x;
default:
break;
}
I have a patch that fixes those up for clang:
https://reviews.llvm.org/D91895
There's 3 other cases that don't quite match between GCC and Clang I
observe in the kernel:
switch (x) {
case 0:
++x;
default:
goto y;
}
y:;
switch (x) {
case 0:
++x;
default:
return;
}
switch (x) {
case 0:
++x;
default:
;
}
Based on your link, and Nathan's comment on my patch, maybe Clang
should continue to warn for the above (at least the `default: return;`
case) and GCC should change? While the last case looks harmless,
there were only 1 or 2 across the tree in my limited configuration
testing; I really think we should just add `break`s for those.
--
Thanks,
~Nick Desaulniers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192u: Remove an unnecessary NULL check
On Tue, May 21, 2019 at 10:42 AM Nathan Chancellor
wrote:
>
> Clang warns:
>
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:2663:47: warning:
> address of array 'param->u.wpa_ie.data' will always evaluate to 'true'
> [-Wpointer-bool-conversion]
> (param->u.wpa_ie.len && !param->u.wpa_ie.data))
> ~^~~~
>
> This was exposed by commit deabe03523a7 ("Staging: rtl8192u: ieee80211:
> Use !x in place of NULL comparisons") because we disable the warning
> that would have pointed out the comparison against NULL is also false:
>
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:2663:46: warning:
> comparison of array 'param->u.wpa_ie.data' equal to a null pointer is
> always false [-Wtautological-pointer-compare]
> (param->u.wpa_ie.len && param->u.wpa_ie.data == NULL))
> ^~~~
>
> Remove it so clang no longer warns.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/487
> Signed-off-by: Nathan Chancellor
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> index f38f9d8b78bb..e0da0900a4f7 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> @@ -2659,8 +2659,7 @@ static int ieee80211_wpa_set_wpa_ie(struct
> ieee80211_device *ieee,
> {
> u8 *buf;
>
> - if (param->u.wpa_ie.len > MAX_WPA_IE_LEN ||
> - (param->u.wpa_ie.len && !param->u.wpa_ie.data))
Right so, the types in this expression:
param: struct ieee_param*
param->u: *anonymous union*
param->u.wpa_ie: *anonymous struct*
param->u.wpa_ie.len: u32
param->u.wpa_ie.data: u8 [0]
as defined in drivers/staging/rtl8192u/ieee80211/ieee80211.h#L295
https://github.com/ClangBuiltLinux/linux/blob/9c7db5004280767566e91a33445bf93aa479ef02/drivers/staging/rtl8192u/ieee80211/ieee80211.h#L295-L322
so this is a tricky case, because in general array members can never
themselves be NULL, and usually I trust -Wpointer-bool-conversion, but
this is a special case because of the flexible array member:
https://en.wikipedia.org/wiki/Flexible_array_member. (It seems that
having the 0 in the length explicitly was pre-c99 GNU extension:
https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html). I wonder if
-Wtautological-pointer-compare applies to Flexible Array Members or
not (Richard, do you know)? In general, you'd be setting
param->u.wpa_ie to the return value of a dynamic memory allocation,
not param->u.wpa_ie.data, so this check is fishy to me.
> + if (param->u.wpa_ie.len > MAX_WPA_IE_LEN)
> return -EINVAL;
>
> if (param->u.wpa_ie.len) {
> --
> 2.22.0.rc1
>
--
Thanks,
~Nick Desaulniers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: kpc2000: Use memset to initialize resources
On Wed, Apr 24, 2019 at 11:58 AM Nathan Chancellor
wrote:
>
> Clang warns:
>
> drivers/staging/kpc2000/kpc2000/cell_probe.c:96:38: warning: suggest
> braces around initialization of subobject [-Wmissing-braces]
> struct resource resources[2] = {0};
> ^
> {}
> drivers/staging/kpc2000/kpc2000/cell_probe.c:314:38: warning: suggest
> braces around initialization of subobject [-Wmissing-braces]
> struct resource resources[2] = {0};
> ^
> {}
> 2 warnings generated.
>
> One way to fix these warnings is to add additional braces like Clang
> suggests; however, there has been a bit of push back from some
> maintainers, who just prefer memset as it is unambiguous, doesn't
> depend on a particular compiler version, and properly initializes all
> subobjects [1][2]. Do that here so there are no more warnings.
>
> [1]:
> https://lore.kernel.org/lkml/022e41c0-8465-dc7a-a45c-64187ecd9...@amd.com/
> [2]:
> https://lore.kernel.org/lkml/20181128.215241.702406654469517539.da...@davemloft.net/
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/455
> Signed-off-by: Nathan Chancellor
> ---
> drivers/staging/kpc2000/kpc2000/cell_probe.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index ad2cc0a3bfa1..13f544f3c0b9 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -93,8 +93,8 @@ void parse_core_table_entry(struct core_table_entry *cte,
> const u64 read_val, co
> int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
> char *name, const struct core_table_entry cte)
> {
> struct mfd_cell cell = {0};
> -struct resource resources[2] = {0};
> -
> +struct resource resources[2];
> +
> struct kpc_core_device_platdata core_pdata = {
> .card_id = pcard->card_id,
> .build_version = pcard->build_version,
> @@ -112,6 +112,8 @@ int probe_core_basic(unsigned int core_num, struct
> kp2000_device *pcard, char *
> cell.id = core_num;
> cell.num_resources = 2;
>
> +memset(, 0, sizeof(resources));
> +
> resources[0].start = cte.offset;
> resources[0].end = cte.offset + (cte.length - 1);
> resources[0].flags = IORESOURCE_MEM;
> @@ -311,8 +313,8 @@ int probe_core_uio(unsigned int core_num, struct
> kp2000_device *pcard, char *na
> static int create_dma_engine_core(struct kp2000_device *pcard, size_t
> engine_regs_offset, int engine_num, int irq_num)
> {
> struct mfd_cell cell = {0};
> -struct resource resources[2] = {0};
> -
> +struct resource resources[2];
> +
> dev_dbg(>pdev->dev, "create_dma_core(pcard = [%p],
> engine_regs_offset = %zx, engine_num = %d)\n", pcard, engine_regs_offset,
> engine_num);
>
> cell.platform_data = NULL;
> @@ -321,6 +323,8 @@ static int create_dma_engine_core(struct kp2000_device
> *pcard, size_t engine_re
> cell.name = KP_DRIVER_NAME_DMA_CONTROLLER;
> cell.num_resources = 2;
>
> +memset(, 0, sizeof(resources));
> +
Bonus points for clearing up the leading whitespace. Thanks for the
patch. I'm beginning to think aggregate initializers are the devil.
Reviewed-by: Nick Desaulniers
> resources[0].start = engine_regs_offset;
> resources[0].end = engine_regs_offset + (KPC_DMA_ENGINE_SIZE - 1);
> resources[0].flags = IORESOURCE_MEM;
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used
On Wed, Jan 9, 2019 at 11:01 PM Greg KH wrote: > > On Wed, Jan 09, 2019 at 04:19:30PM -0800, Nick Desaulniers wrote: > > Digging up an old thread to point out I was wrong: > > > > On Thu, Sep 27, 2018 at 1:08 PM Nick Desaulniers > > wrote: > > > > This is not a million-little-commits problem; more like 2 drivers in > > > > the whole tree have this problem. > > > > > > > > Note that this change that Nathan has DOES NOT need to applied to > > > > every driver that uses MODULE_DEVICE_TABLE. Only when the struct has > > > > no other references other than JUST being passed to > > > > MODULE_DEVICE_TABLE() is this a problem. For an allyesconfig, I only > > > > see 2 instances of this case in the whole tree: > > > > > > > > https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is > > > > addressing > > > > https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the > > > > results of this patch > > > > > > > > If you're ok with this change to two drivers, then: > > > > Reviewed-by: Nick Desaulniers > > > > I just landed a fix for this in Clang > > (https://reviews.llvm.org/rL350776) which will ship soon in Clang 8. > > Sedat helped test this patch and reported that it removed 41 > > false-negative warnings > > https://github.com/ClangBuiltLinux/linux/issues/232#issuecomment-440006399 > > Great, thanks for letting me know. Can I revert this patch? Go for it. I think my mistake was that we had 2 instances of this listed in our bug tracker, but an allyesconfig had many many more instances than I was aware of. -- Thanks, ~Nick Desaulniers ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as maybe unused
On Wed, Sep 26, 2018 at 11:39 AM Greg KH wrote:
>
> On Wed, Sep 26, 2018 at 11:28:46AM -0700, Nick Desaulniers wrote:
> > On Wed, Sep 26, 2018 at 10:55 AM Nick Desaulniers
> > wrote:
> > >
> > > On Wed, Sep 26, 2018 at 12:41 AM Nathan Chancellor
> > > wrote:
> > > >
> > > > On Wed, Sep 26, 2018 at 09:13:59AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 26, 2018 at 12:02:09AM -0700, Nathan Chancellor wrote:
> > > > > > Clang emits the following warning:
> > > > > >
> > > > > > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning:
> > > > > > variable
> > > > > > 'acpi_ids' is not needed and will not be emitted
> > > > > > [-Wunneeded-internal-declaration]
> > > > > > static const struct acpi_device_id acpi_ids[] = {
> > > > > >^
> > > > > > 1 warning generated.
> > > > > >
> > > > > > Mark the declaration as maybe unused like a few other instances of
> > > > > > this
> > > > > > construct in the kernel.
> > > > > >
> > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/169
> > > > > > Signed-off-by: Nathan Chancellor
> > > > > > ---
> > > > > > drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > > > b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > > > index 6d02904de63f..3285bf36291b 100644
> > > > > > --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > > > +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > > > @@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
> > > > > > { SDIO_DEVICE(0x024c, 0xb723), },
> > > > > > { /* end: all zeroes */ },
> > > > > > };
> > > > > > -static const struct acpi_device_id acpi_ids[] = {
> > > > > > +static const struct acpi_device_id acpi_ids[] __maybe_unused = {
> > > > >
> > > > > But it is used. No "maybe" at all here. The MODULE_DEVICE_TABLE()
> > > > > macro does a functional thing. Why is gcc not reporting an issue with
> > > > > this and clang is?
> > >
> > > Looks like GCC and Clang handle __attribute__((alias)) differently
> > > when optimizations are enabled. Clang has
> > > -Wunneeded-internal-declaration (GCC doesn't have that specific flag,
> > > which is why GCC doesn't report, but its behavior is also different),
> > > as part of -Wall, which warns that it thinks the static var is dead,
> > > then removes it from -O1 and up.
> > >
> > > https://godbolt.org/z/Dpxnbp
> > >
> > > I consider this a bug in Clang:
> > > https://bugs.llvm.org/show_bug.cgi?id=39088
> > >
> > > As Nathan notes in this comment in V1:
> > > https://lore.kernel.org/lkml/20180926064437.GA29417@flashbox/ having
> > > additional references to the static array is enough to keep it around.
> > > When there are no other references, then it should not be marked
> > > static, in order to still be emitted.
> > >
> > > We can work around this by removing `static` from struct definitions
> > > that no other references than being passed to the MODULE_DEVICE_TABLE
> > > macro. GCC and Clang will then both emit references to both
> > > identifiers.
> >
> > Sorry, after thinking more about this and discussing more with a
> > coworker, I think Clang is doing the right thing, and that `static`
> > should be removed from declarations passed to MODULE_DEVICE_TABLE
> > without other references. Clang's Dead Code Elimination (DCE) here
> > seems to be more aggressive, but it still looks correct to me.
> >
> > int foo [] = { 42, 0xDEAD }; // extern
> > extern typeof(foo) bar __attribute__((unused, alias("foo")));
> >
> > GCC and Clang at -O2 both produce references to foo and bar.
> >
> > Adding `static` to foo, then GCC produces both references, while Clang
> > moves the data to bar and removes foo. This is safe because foo has
> > no other references, and bar is what file2alias.c cares about in this
> > ca
Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as maybe unused
On Wed, Sep 26, 2018 at 10:55 AM Nick Desaulniers
wrote:
>
> On Wed, Sep 26, 2018 at 12:41 AM Nathan Chancellor
> wrote:
> >
> > On Wed, Sep 26, 2018 at 09:13:59AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 26, 2018 at 12:02:09AM -0700, Nathan Chancellor wrote:
> > > > Clang emits the following warning:
> > > >
> > > > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning: variable
> > > > 'acpi_ids' is not needed and will not be emitted
> > > > [-Wunneeded-internal-declaration]
> > > > static const struct acpi_device_id acpi_ids[] = {
> > > >^
> > > > 1 warning generated.
> > > >
> > > > Mark the declaration as maybe unused like a few other instances of this
> > > > construct in the kernel.
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/169
> > > > Signed-off-by: Nathan Chancellor
> > > > ---
> > > > drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > index 6d02904de63f..3285bf36291b 100644
> > > > --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > > @@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
> > > > { SDIO_DEVICE(0x024c, 0xb723), },
> > > > { /* end: all zeroes */ },
> > > > };
> > > > -static const struct acpi_device_id acpi_ids[] = {
> > > > +static const struct acpi_device_id acpi_ids[] __maybe_unused = {
> > >
> > > But it is used. No "maybe" at all here. The MODULE_DEVICE_TABLE()
> > > macro does a functional thing. Why is gcc not reporting an issue with
> > > this and clang is?
>
> Looks like GCC and Clang handle __attribute__((alias)) differently
> when optimizations are enabled. Clang has
> -Wunneeded-internal-declaration (GCC doesn't have that specific flag,
> which is why GCC doesn't report, but its behavior is also different),
> as part of -Wall, which warns that it thinks the static var is dead,
> then removes it from -O1 and up.
>
> https://godbolt.org/z/Dpxnbp
>
> I consider this a bug in Clang: https://bugs.llvm.org/show_bug.cgi?id=39088
>
> As Nathan notes in this comment in V1:
> https://lore.kernel.org/lkml/20180926064437.GA29417@flashbox/ having
> additional references to the static array is enough to keep it around.
> When there are no other references, then it should not be marked
> static, in order to still be emitted.
>
> We can work around this by removing `static` from struct definitions
> that no other references than being passed to the MODULE_DEVICE_TABLE
> macro. GCC and Clang will then both emit references to both
> identifiers.
Sorry, after thinking more about this and discussing more with a
coworker, I think Clang is doing the right thing, and that `static`
should be removed from declarations passed to MODULE_DEVICE_TABLE
without other references. Clang's Dead Code Elimination (DCE) here
seems to be more aggressive, but it still looks correct to me.
int foo [] = { 42, 0xDEAD }; // extern
extern typeof(foo) bar __attribute__((unused, alias("foo")));
GCC and Clang at -O2 both produce references to foo and bar.
Adding `static` to foo, then GCC produces both references, while Clang
moves the data to bar and removes foo. This is safe because foo has
no other references, and bar is what file2alias.c cares about in this
case.
--
Thanks,
~Nick Desaulniers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
