linux-kernel - Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as maybe unused

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOdkkaCAmizfOr8Bj8QLTN161KqwXRCbuKbiNDoo5RonrpQ@mail.gmail.com>
Date:   Wed, 26 Sep 2018 10:55:07 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>, devel@...verdev.osuosl.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as maybe unused

On Wed, Sep 26, 2018 at 12:41 AM Nathan Chancellor
<natechancellor@...il.com> 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 <natechancellor@...il.com>
> > > ---
> > >  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.
-- 
Thanks,
~Nick Desaulniers