| 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
| ||
|
Message-ID: <CAK7LNAQRu0p8_8DqEdmUUPfc4gC60SSj90vTpz3iKaiE596-TA@mail.gmail.com>
Date: Thu, 21 Jun 2018 08:21:01 +0900
From: Masahiro Yamada <yamada.masahiro@...ionext.com>
To: Paul Burton <paul.burton@...s.com>
Cc: Arnd Bergmann <arnd@...db.de>,
Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Linux-MIPS <linux-mips@...ux-mips.org>,
Ingo Molnar <mingo@...nel.org>,
Matthew Wilcox <matthew@....cx>,
Thomas Gleixner <tglx@...utronix.de>,
Douglas Anderson <dianders@...omium.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Matthias Kaehlcke <mka@...omium.org>,
He Zhe <zhe.he@...driver.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Michal Marek <michal.lkml@...kovi.net>,
Khem Raj <raj.khem@...il.com>,
Christophe Leroy <christophe.leroy@....fr>,
Al Viro <viro@...iv.linux.org.uk>,
Stafford Horne <shorne@...il.com>,
Gideon Israel Dsouza <gidisrael@...il.com>,
Kees Cook <keescook@...omium.org>,
Michael Ellerman <mpe@...erman.id.au>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Paul Mackerras <paulus@...ba.org>,
linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH 1/3] kbuild: add macro for controlling warnings to linux/compiler.h
2018-06-20 4:02 GMT+09:00 Paul Burton <paul.burton@...s.com>:
> Hi Masahiro,
>
> On Wed, Jun 20, 2018 at 02:34:35AM +0900, Masahiro Yamada wrote:
>> 2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@...s.com>:
>> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> > index f1a7492a5cc8..aba64a2912d8 100644
>> > --- a/include/linux/compiler-gcc.h
>> > +++ b/include/linux/compiler-gcc.h
>> > @@ -347,3 +347,69 @@
>> > #if GCC_VERSION >= 50100
>> > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
>> > #endif
>> > +
>> > +/*
>> > + * turn individual warnings and errors on and off locally, depending
>> > + * on version.
>> > + */
>> > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s)
>> > +
>> > +#if GCC_VERSION >= 40600
>> > +#define __diag_str1(s) #s
>> > +#define __diag_str(s) __diag_str1(s)
>> > +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))
>> > +
>> > +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
>> > +#define __diag_GCC_4_6(s) __diag(s)
>> > +#else
>> > +#define __diag(s)
>> > +#define __diag_GCC_4_6(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 40700
>> > +#define __diag_GCC_4_7(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_4_7(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 40800
>> > +#define __diag_GCC_4_8(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_4_8(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 40900
>> > +#define __diag_GCC_4_9(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_4_9(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 50000
>> > +#define __diag_GCC_5(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_5(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 60000
>> > +#define __diag_GCC_6(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_6(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 70000
>> > +#define __diag_GCC_7(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_7(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 80000
>> > +#define __diag_GCC_8(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_8(s)
>> > +#endif
>> > +
>> > +#if GCC_VERSION >= 90000
>> > +#define __diag_GCC_9(s) __diag(s)
>> > +#else
>> > +#define __diag_GCC_9(s)
>> > +#endif
>>
>>
>> Hmm, we would have to add this for every release.
>
> Well, strictly speaking only ones that we need to modify diags for - ie.
> in this series we could get away with only adding the GCC 8 macro if we
> wanted.
>
>> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
>> > index 6b79a9bba9a7..313a2ad884e0 100644
>> > --- a/include/linux/compiler_types.h
>> > +++ b/include/linux/compiler_types.h
>> > @@ -271,4 +271,22 @@ struct ftrace_likely_data {
>> > # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
>> > #endif
>> >
>> > +#ifndef __diag
>> > +#define __diag(string)
>> > +#endif
>> > +
>> > +#ifndef __diag_GCC
>> > +#define __diag_GCC(string)
>> > +#endif
>>
>> __diag_GCC() takes two arguments,
>> so it should be:
>>
>> #ifndef __diag_GCC
>> #define __diag_GCC(version, s)
>> #endif
>>
>>
>> Otherwise, this would cause warning like this:
>>
>>
>> arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2
>> arguments, but takes just 1
>> SYSCALL_DEFINE1(arm64_personality, unsigned int, personality)
>> ^~~~~~~~~~
>
> Yes, good catch.
>
>> > +#define __diag_push() __diag(push)
>> > +#define __diag_pop() __diag(pop)
>> > +
>> > +#define __diag_ignore(compiler, version, option, comment) \
>> > + __diag_ ## compiler(version, ignored option)
>> > +#define __diag_warn(compiler, version, option, comment) \
>> > + __diag_ ## compiler(version, warning option)
>> > +#define __diag_error(compiler, version, option, comment) \
>> > + __diag_ ## compiler(version, error option)
>> > +
>>
>> To me, it looks like this is putting GCC/Clang specific things
>> in the common file, <linux/compiler_types.h> .
>>
>> All compilers must use "ignored", "warning", "error",
>> not allowed to use "ignore".
>
> We could move that to linux/compiler-gcc.h pretty easily.
>
>> I also wonder if we could avoid proliferating __diag_GCC_*.
>
> My thought is that it's unlikely we'll ever support enough different
> compilers for it to become problematic to list the ones we modify
> warnings for in linux/compiler_types.h.
>
>> I attached a bit different implementation below.
>>
>> I used -Wno-pragmas to avoid unknown option warnings.
>
> That doesn't seem very clean to me because it will hide typos or other
> mistakes. One advantage of Arnd's patch is that by specifying the
> compiler & version we only attempt to use pragmas that are appropriate
> so we don't need to ignore unknown ones.
>
>> Usage is
>>
>> __diag_push();
>> __diag_ignore(-Wattribute-alias,
>> "Type aliasing is used to sanitize syscall arguments");
>> ...
>> __diag_pop();
>>
>> Comments, ideas are appreciated.
>
> By removing the compiler & version arguments you're enforcing that if we
> ignore a warning for one compiler we must ignore it for all, regardless
> of whether it's problematic. This essentially presumes that warnings
> with the same name will behave the same across compilers, which feels
> worse to me than having users of this explicitly state which compilers
> need the pragmas.
Fair enough.
V2 is good except one nit.
(I left a comment in it)
I can fix it up locally
if it is tedious to re-spin, though.
> Thanks,
> Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
Powered by blists - more mailing lists