[PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Miguel Ojeda]

The upcoming GCC 9 release extends the -Wmissing-attributes warnings
(enabled by -Wall) to C and aliases: it warns when particular function
attributes are missing in the aliases but not in their target, e.g.:

    void __cold f(void) {}
    void __alias("f") g(void);

diagnoses:

    warning: 'g' specifies less restrictive attribute than
    its target 'f': 'cold' [-Wmissing-attributes]

These patch series clean these new warnings. Most of them are caused
by the module_init/exit macros.

The first patch has been in -next for a long time already, and an alternative
solution (only __cold) for module.h as well. However, since we decided
to go with the new __copy attribute, I will leave the series for a few days
again and send the PR for -rc7.

Link: https://lore.kernel.org/lkml/20190125104353.2791-1-labbott@redhat.com/

Miguel Ojeda (3):
  lib/crc32.c: mark crc32_le_base/__crc32c_le_base aliases as __pure
  Compiler Attributes: add support for __copy (gcc >= 9)
  include/linux/module.h: copy __init/__exit attrs to
    init/cleanup_module

 include/linux/compiler_attributes.h | 14 ++++++++++++++
 include/linux/module.h              |  4 ++--
 lib/crc32.c                         |  4 ++--
 3 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/3] lib/crc32.c: mark crc32_le_base/__crc32c_le_base aliases as __pure

[Miguel Ojeda]

The upcoming GCC 9 release extends the -Wmissing-attributes warnings
(enabled by -Wall) to C and aliases: it warns when particular function
attributes are missing in the aliases but not in their target.

In particular, it triggers here because crc32_le_base/__crc32c_le_base
aren't __pure while their target crc32_le/__crc32c_le are.

These aliases are used by architectures as a fallback in accelerated
versions of CRC32. See commit 9784d82db3eb ("lib/crc32: make core crc32()
routines weak so they can be overridden").

Therefore, being fallbacks, it is likely that even if the aliases
were called from C, there wouldn't be any optimizations possible.
Currently, the only user is arm64, which calls this from asm.

Still, marking the aliases as __pure makes sense and is a good idea
for documentation purposes and possible future optimizations,
which also silences the warning.

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Laura Abbott <labbott@redhat.com>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 lib/crc32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/crc32.c b/lib/crc32.c
index 45b1d67a1767..4a20455d1f61 100644
--- a/lib/crc32.c
+++ b/lib/crc32.c
@@ -206,8 +206,8 @@ u32 __pure __weak __crc32c_le(u32 crc, unsigned char const *p, size_t len)
 EXPORT_SYMBOL(crc32_le);
 EXPORT_SYMBOL(__crc32c_le);
 
-u32 crc32_le_base(u32, unsigned char const *, size_t) __alias(crc32_le);
-u32 __crc32c_le_base(u32, unsigned char const *, size_t) __alias(__crc32c_le);
+u32 __pure crc32_le_base(u32, unsigned char const *, size_t) __alias(crc32_le);
+u32 __pure __crc32c_le_base(u32, unsigned char const *, size_t) __alias(__crc32c_le);
 
 /*
  * This multiplies the polynomials x and y modulo the given modulus.
-- 
2.17.1

[PATCH 2/3] Compiler Attributes: add support for __copy (gcc >= 9)

[Miguel Ojeda]

From the GCC manual:

  copy
  copy(function)

    The copy attribute applies the set of attributes with which function
    has been declared to the declaration of the function to which
    the attribute is applied. The attribute is designed for libraries
    that define aliases or function resolvers that are expected
    to specify the same set of attributes as their targets. The copy
    attribute can be used with functions, variables, or types. However,
    the kind of symbol to which the attribute is applied (either
    function or variable) must match the kind of symbol to which
    the argument refers. The copy attribute copies only syntactic and
    semantic attributes but not attributes that affect a symbol’s
    linkage or visibility such as alias, visibility, or weak.
    The deprecated attribute is also not copied.

  https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

The upcoming GCC 9 release extends the -Wmissing-attributes warnings
(enabled by -Wall) to C and aliases: it warns when particular function
attributes are missing in the aliases but not in their target, e.g.:

    void __cold f(void) {}
    void __alias("f") g(void);

diagnoses:

    warning: 'g' specifies less restrictive attribute than
    its target 'f': 'cold' [-Wmissing-attributes]

Using __copy(f) we can copy the __cold attribute from f to g:

    void __cold f(void) {}
    void __copy(f) __alias("f") g(void);

This attribute is most useful to deal with situations where an alias
is declared but we don't know the exact attributes the target has.

For instance, in the kernel, the widely used module_init/exit macros
define the init/cleanup_module aliases, but those cannot be marked
always as __init/__exit since they some modules do not have their
functions marked as such.

Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 include/linux/compiler_attributes.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 19f32b0c29af..6b318efd8a74 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -34,6 +34,7 @@
 #ifndef __has_attribute
 # define __has_attribute(x) __GCC4_has_attribute_##x
 # define __GCC4_has_attribute___assume_aligned__      (__GNUC_MINOR__ >= 9)
+# define __GCC4_has_attribute___copy__                0
 # define __GCC4_has_attribute___designated_init__     0
 # define __GCC4_has_attribute___externally_visible__  1
 # define __GCC4_has_attribute___noclone__             1
@@ -100,6 +101,19 @@
  */
 #define __attribute_const__             __attribute__((__const__))
 
+/*
+ * Optional: only supported since gcc >= 9
+ * Optional: not supported by clang
+ * Optional: not supported by icc
+ *
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-copy-function-attribute
+ */
+#if __has_attribute(__copy__)
+# define __copy(symbol)                 __attribute__((__copy__(symbol)))
+#else
+# define __copy(symbol)
+#endif
+
 /*
  * Don't. Just don't. See commit 771c035372a0 ("deprecate the '__deprecated'
  * attribute warnings entirely and for good") for more information.
-- 
2.17.1

Re: [PATCH 2/3] Compiler Attributes: add support for __copy (gcc >= 9)

[Nick Desaulniers]

On Fri, Feb 8, 2019 at 4:09 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> From the GCC manual:
>
>   copy
>   copy(function)
>
>     The copy attribute applies the set of attributes with which function
>     has been declared to the declaration of the function to which
>     the attribute is applied. The attribute is designed for libraries
>     that define aliases or function resolvers that are expected
>     to specify the same set of attributes as their targets. The copy
>     attribute can be used with functions, variables, or types. However,
>     the kind of symbol to which the attribute is applied (either
>     function or variable) must match the kind of symbol to which
>     the argument refers. The copy attribute copies only syntactic and
>     semantic attributes but not attributes that affect a symbol’s
>     linkage or visibility such as alias, visibility, or weak.
>     The deprecated attribute is also not copied.
>
>   https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
>
> The upcoming GCC 9 release extends the -Wmissing-attributes warnings
> (enabled by -Wall) to C and aliases: it warns when particular function
> attributes are missing in the aliases but not in their target, e.g.:
>
>     void __cold f(void) {}
>     void __alias("f") g(void);
>
> diagnoses:
>
>     warning: 'g' specifies less restrictive attribute than
>     its target 'f': 'cold' [-Wmissing-attributes]
>
> Using __copy(f) we can copy the __cold attribute from f to g:
>
>     void __cold f(void) {}
>     void __copy(f) __alias("f") g(void);
>
> This attribute is most useful to deal with situations where an alias
> is declared but we don't know the exact attributes the target has.
>
> For instance, in the kernel, the widely used module_init/exit macros
> define the init/cleanup_module aliases, but those cannot be marked
> always as __init/__exit since they some modules do not have their
> functions marked as such.

Drop "they" from this sentence if there's a respin, otherwise looks helpful.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> ---
>  include/linux/compiler_attributes.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 19f32b0c29af..6b318efd8a74 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -34,6 +34,7 @@
>  #ifndef __has_attribute
>  # define __has_attribute(x) __GCC4_has_attribute_##x
>  # define __GCC4_has_attribute___assume_aligned__      (__GNUC_MINOR__ >= 9)
> +# define __GCC4_has_attribute___copy__                0
>  # define __GCC4_has_attribute___designated_init__     0
>  # define __GCC4_has_attribute___externally_visible__  1
>  # define __GCC4_has_attribute___noclone__             1
> @@ -100,6 +101,19 @@
>   */
>  #define __attribute_const__             __attribute__((__const__))
>
> +/*
> + * Optional: only supported since gcc >= 9
> + * Optional: not supported by clang
> + * Optional: not supported by icc
> + *
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-copy-function-attribute
> + */
> +#if __has_attribute(__copy__)
> +# define __copy(symbol)                 __attribute__((__copy__(symbol)))
> +#else
> +# define __copy(symbol)
> +#endif
> +
>  /*
>   * Don't. Just don't. See commit 771c035372a0 ("deprecate the '__deprecated'
>   * attribute warnings entirely and for good") for more information.
> --
> 2.17.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/3] Compiler Attributes: add support for __copy (gcc >= 9)

[Miguel Ojeda]

On Sat, Feb 9, 2019 at 1:41 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Drop "they" from this sentence if there's a respin, otherwise looks helpful.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Good catch!

Thank you,
Miguel

[PATCH 3/3] include/linux/module.h: copy __init/__exit attrs to init/cleanup_module

[Miguel Ojeda]

The upcoming GCC 9 release extends the -Wmissing-attributes warnings
(enabled by -Wall) to C and aliases: it warns when particular function
attributes are missing in the aliases but not in their target.

In particular, it triggers for all the init/cleanup_module
aliases in the kernel (defined by the module_init/exit macros),
ending up being very noisy.

These aliases point to the __init/__exit functions of a module,
which are defined as __cold (among other attributes). However,
the aliases themselves do not have the __cold attribute.

Since the compiler behaves differently when compiling a __cold
function as well as when compiling paths leading to calls
to __cold functions, the warning is trying to point out
the possibly-forgotten attribute in the alias.

In order to keep the warning enabled, we decided to silence
this case. Ideally, we would mark the aliases directly
as __init/__exit. However, there are currently around 132 modules
in the kernel which are missing __init/__exit in their init/cleanup
functions (either because they are missing, or for other reasons,
e.g. the functions being called from somewhere else); and
a section mismatch is a hard error.

A conservative alternative was to mark the aliases as __cold only.
However, since we would like to eventually enforce __init/__exit
to be always marked,  we chose to use the new __copy function
attribute (introduced by GCC 9 as well to deal with this).
With it, we copy the attributes used by the target functions
into the aliases. This way, functions that were not marked
as __init/__exit won't have their aliases marked either,
and therefore there won't be a section mismatch.

Note that the warning would go away marking either the extern
declaration, the definition, or both. However, we only mark
the definition of the alias, since we do not want callers
(which only see the declaration) to be compiled as if the function
was __cold (and therefore the paths leading to those calls
would be assumed to be unlikely).

Link: https://lore.kernel.org/lkml/20190123173707.GA16603@gmail.com/
Link: https://lore.kernel.org/lkml/20190206175627.GA20399@gmail.com/
Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 include/linux/module.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 8fa38d3e7538..f5bc4c046461 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -129,13 +129,13 @@ extern void cleanup_module(void);
 #define module_init(initfn)					\
 	static inline initcall_t __maybe_unused __inittest(void)		\
 	{ return initfn; }					\
-	int init_module(void) __attribute__((alias(#initfn)));
+	int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)					\
 	static inline exitcall_t __maybe_unused __exittest(void)		\
 	{ return exitfn; }					\
-	void cleanup_module(void) __attribute__((alias(#exitfn)));
+	void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
 
 #endif
 
-- 
2.17.1

Re: [PATCH 3/3] include/linux/module.h: copy __init/__exit attrs to init/cleanup_module

[Jessica Yu]

+++ Miguel Ojeda [09/02/19 01:08 +0100]:
>The upcoming GCC 9 release extends the -Wmissing-attributes warnings
>(enabled by -Wall) to C and aliases: it warns when particular function
>attributes are missing in the aliases but not in their target.
>
>In particular, it triggers for all the init/cleanup_module
>aliases in the kernel (defined by the module_init/exit macros),
>ending up being very noisy.
>
>These aliases point to the __init/__exit functions of a module,
>which are defined as __cold (among other attributes). However,
>the aliases themselves do not have the __cold attribute.
>
>Since the compiler behaves differently when compiling a __cold
>function as well as when compiling paths leading to calls
>to __cold functions, the warning is trying to point out
>the possibly-forgotten attribute in the alias.
>
>In order to keep the warning enabled, we decided to silence
>this case. Ideally, we would mark the aliases directly
>as __init/__exit. However, there are currently around 132 modules
>in the kernel which are missing __init/__exit in their init/cleanup
>functions (either because they are missing, or for other reasons,
>e.g. the functions being called from somewhere else); and
>a section mismatch is a hard error.
>
>A conservative alternative was to mark the aliases as __cold only.
>However, since we would like to eventually enforce __init/__exit
>to be always marked,  we chose to use the new __copy function
>attribute (introduced by GCC 9 as well to deal with this).
>With it, we copy the attributes used by the target functions
>into the aliases. This way, functions that were not marked
>as __init/__exit won't have their aliases marked either,
>and therefore there won't be a section mismatch.
>
>Note that the warning would go away marking either the extern
>declaration, the definition, or both. However, we only mark
>the definition of the alias, since we do not want callers
>(which only see the declaration) to be compiled as if the function
>was __cold (and therefore the paths leading to those calls
>would be assumed to be unlikely).
>
>Link: https://lore.kernel.org/lkml/20190123173707.GA16603@gmail.com/
>Link: https://lore.kernel.org/lkml/20190206175627.GA20399@gmail.com/
>Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
>Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

Acked-by: Jessica Yu <jeyu@kernel.org>

Thanks!

>---
> include/linux/module.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 8fa38d3e7538..f5bc4c046461 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -129,13 +129,13 @@ extern void cleanup_module(void);
> #define module_init(initfn)					\
> 	static inline initcall_t __maybe_unused __inittest(void)		\
> 	{ return initfn; }					\
>-	int init_module(void) __attribute__((alias(#initfn)));
>+	int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
>
> /* This is only required if you want to be unloadable. */
> #define module_exit(exitfn)					\
> 	static inline exitcall_t __maybe_unused __exittest(void)		\
> 	{ return exitfn; }					\
>-	void cleanup_module(void) __attribute__((alias(#exitfn)));
>+	void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
>
> #endif
>
>-- 
>2.17.1
>

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Ard Biesheuvel]

On Sat, 9 Feb 2019 at 01:09, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> The upcoming GCC 9 release extends the -Wmissing-attributes warnings
> (enabled by -Wall) to C and aliases: it warns when particular function
> attributes are missing in the aliases but not in their target, e.g.:
>
>     void __cold f(void) {}

Apologies if I missed any discussions on this topic, but I would argue
that 'cold' is not an attribute that has to be applied to the function
pointer as well as each of its targets, since it basically decides the
placement in the binary, and it doesn't affect the nature of the code
being referenced.

Permitting a function pointer to point to 'cold' functions only if it
is annotated as 'cold' itself makes no sense whatsoever, and there
could be valid cases where a function pointer may point at either cold
or non-cold functions.

For other attributes, the situation is clearly different, e.g., a
__pure function pointer to a non-pure function is obviously a bug,
since the compiler could make assumptions based on the function
pointer type that do not hold for the function it points to at
runtime.

I don't see how the 'cold' attribute could have any such effect, so I
wonder if it isn't simply a bug that we have to report to GCC?



>     void __alias("f") g(void);
>
> diagnoses:
>
>     warning: 'g' specifies less restrictive attribute than
>     its target 'f': 'cold' [-Wmissing-attributes]
>
> These patch series clean these new warnings. Most of them are caused
> by the module_init/exit macros.
>
> The first patch has been in -next for a long time already, and an alternative
> solution (only __cold) for module.h as well. However, since we decided
> to go with the new __copy attribute, I will leave the series for a few days
> again and send the PR for -rc7.
>
> Link: https://lore.kernel.org/lkml/20190125104353.2791-1-labbott@redhat.com/
>
> Miguel Ojeda (3):
>   lib/crc32.c: mark crc32_le_base/__crc32c_le_base aliases as __pure
>   Compiler Attributes: add support for __copy (gcc >= 9)
>   include/linux/module.h: copy __init/__exit attrs to
>     init/cleanup_module
>
>  include/linux/compiler_attributes.h | 14 ++++++++++++++
>  include/linux/module.h              |  4 ++--
>  lib/crc32.c                         |  4 ++--
>  3 files changed, 18 insertions(+), 4 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Miguel Ojeda]

On Sat, Feb 9, 2019 at 11:44 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 9 Feb 2019 at 01:09, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > The upcoming GCC 9 release extends the -Wmissing-attributes warnings
> > (enabled by -Wall) to C and aliases: it warns when particular function
> > attributes are missing in the aliases but not in their target, e.g.:
> >
> >     void __cold f(void) {}
>
> Apologies if I missed any discussions on this topic, but I would argue
> that 'cold' is not an attribute that has to be applied to the function
> pointer as well as each of its targets, since it basically decides the
> placement in the binary, and it doesn't affect the nature of the code
> being referenced.

It also affects the optimizer in two different ways AFAIK:

  * For the function itself, it gets optimized for size instead of speed.
  * For callers, the paths that lead to the calls are treated as unlikely.

So GCC reports it because you would be (likely) missing the
optimizations you expected if you are using the alias instead of the
target.

In our case in patch 3, we do not want the optimization for callers,
which is why we don't mark the extern declaration as __cold (see the
commit message).

Cheers,
Miguel

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Ard Biesheuvel]

On Sat, 9 Feb 2019 at 12:19, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Feb 9, 2019 at 11:44 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Sat, 9 Feb 2019 at 01:09, Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > The upcoming GCC 9 release extends the -Wmissing-attributes warnings
> > > (enabled by -Wall) to C and aliases: it warns when particular function
> > > attributes are missing in the aliases but not in their target, e.g.:
> > >
> > >     void __cold f(void) {}
> >
> > Apologies if I missed any discussions on this topic, but I would argue
> > that 'cold' is not an attribute that has to be applied to the function
> > pointer as well as each of its targets, since it basically decides the
> > placement in the binary, and it doesn't affect the nature of the code
> > being referenced.
>
> It also affects the optimizer in two different ways AFAIK:
>
>   * For the function itself, it gets optimized for size instead of speed.
>   * For callers, the paths that lead to the calls are treated as unlikely.
>

That seems reasonable, but that still does not mean it is necessarily
a problem if you apply 'cold' to one but not the other.

So to me, it makes perfect sense to permit 'cold' on the target, but
not on the function pointer itself, in which case you get 1) but not
2)

> So GCC reports it because you would be (likely) missing the
> optimizations you expected if you are using the alias instead of the
> target.
>

I see how that could be reasonable for extern declarations that do not
match the definition, since in that case, it is assumed that there is
only one instance of the function. For function pointers, I don't
think this assumption is valid.

> In our case in patch 3, we do not want the optimization for callers,
> which is why we don't mark the extern declaration as __cold (see the
> commit message).
>

You did not cc me on the whole set, so I don't have the patch. But in
any case, GCC 9 has not been released so we should still have time to
talk sense into the GCC guys.

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Miguel Ojeda]

On Sat, Feb 9, 2019 at 12:26 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 9 Feb 2019 at 12:19, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > It also affects the optimizer in two different ways AFAIK:
> >
> >   * For the function itself, it gets optimized for size instead of speed.
> >   * For callers, the paths that lead to the calls are treated as unlikely.
> >
>
> That seems reasonable, but that still does not mean it is necessarily
> a problem if you apply 'cold' to one but not the other.

Indeed. As I said, it is likely that you missed the attribute, not a
sure thing (i.e. that you didn't do it explicitly).

> > So GCC reports it because you would be (likely) missing the
> > optimizations you expected if you are using the alias instead of the
> > target.
> >
>
> I see how that could be reasonable for extern declarations that do not
> match the definition, since in that case, it is assumed that there is
> only one instance of the function. For function pointers, I don't
> think this assumption is valid.

It sounds reasonable to have another warning for
declarations-definition attribute mismatches too. However, I don't see
why you would warn differently. Even if you have one instance of the
function, you may also be using the declaration to explicitly avoid
the unlikely treatment.

Now, whether the warning is worth or not or at which "level", it
depends. I guess the rationale behind having it under -Wall is that
they expect people to have missed copying the attributes, rather than
they are using aliases specifically to avoid a cold/... attribute.

> > In our case in patch 3, we do not want the optimization for callers,
> > which is why we don't mark the extern declaration as __cold (see the
> > commit message).
> >
>
> You did not cc me on the whole set, so I don't have the patch. But in
> any case, GCC 9 has not been released so we should still have time to
> talk sense into the GCC guys.

I only CC'd people on the relevant patches according to
get_maintainers (but yeah, 2 & 3 are related, I could have merged
those lists). Anyway, using the lore.kernel.org server makes it easy
to see an entire series when you have already one:

  https://lore.kernel.org/lkml/20190209000840.11018-1-miguel.ojeda.sandonis@gmail.com/

As for GCC, Martin (the author of the features) is CC'd, so he can
chime in (and I am sure he appreciates the feedback :-)

Cheers,
Miguel

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Martin Sebor]

On 2/9/19 5:31 AM, Miguel Ojeda wrote:
> On Sat, Feb 9, 2019 at 12:26 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> On Sat, 9 Feb 2019 at 12:19, Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>
>>> It also affects the optimizer in two different ways AFAIK:
>>>
>>>    * For the function itself, it gets optimized for size instead of speed.
>>>    * For callers, the paths that lead to the calls are treated as unlikely.
>>>
>>
>> That seems reasonable, but that still does not mean it is necessarily
>> a problem if you apply 'cold' to one but not the other.
> 
> Indeed. As I said, it is likely that you missed the attribute, not a
> sure thing (i.e. that you didn't do it explicitly).
> 
>>> So GCC reports it because you would be (likely) missing the
>>> optimizations you expected if you are using the alias instead of the
>>> target.
>>>
>>
>> I see how that could be reasonable for extern declarations that do not
>> match the definition, since in that case, it is assumed that there is
>> only one instance of the function. For function pointers, I don't
>> think this assumption is valid.
> 
> It sounds reasonable to have another warning for
> declarations-definition attribute mismatches too. However, I don't see
> why you would warn differently. Even if you have one instance of the
> function, you may also be using the declaration to explicitly avoid
> the unlikely treatment.
> 
> Now, whether the warning is worth or not or at which "level", it
> depends. I guess the rationale behind having it under -Wall is that
> they expect people to have missed copying the attributes, rather than
> they are using aliases specifically to avoid a cold/... attribute.
> 
>>> In our case in patch 3, we do not want the optimization for callers,
>>> which is why we don't mark the extern declaration as __cold (see the
>>> commit message).
>>>
>>
>> You did not cc me on the whole set, so I don't have the patch. But in
>> any case, GCC 9 has not been released so we should still have time to
>> talk sense into the GCC guys.
> 
> I only CC'd people on the relevant patches according to
> get_maintainers (but yeah, 2 & 3 are related, I could have merged
> those lists). Anyway, using the lore.kernel.org server makes it easy
> to see an entire series when you have already one:
> 
>    https://lore.kernel.org/lkml/20190209000840.11018-1-miguel.ojeda.sandonis@gmail.com/
> 
> As for GCC, Martin (the author of the features) is CC'd, so he can
> chime in (and I am sure he appreciates the feedback :-)

I do very much, thank you.  I think you've fielded all the GCC
questions here so I don't have much to add.  Just a comment regarding
aliases with different sets of attributes than their targets: it is
possible to come up with use cases not just with attribute cold but
others as well, including mutually exclusive pairs such as const and
pure.  But the uses cases that drove the design of the warning were
those where the set of attributes is meant to be the same, and we
are yet to to come across the others in practice.  If it turns out
that there are others in common use we can tweak the warning logic.

Regarding function pointers, attribute cold only applies to function
declarations (and labels) and is ignored on pointers, so calls to
a cold function through a pointer are not necessarily subject to
the same effects as direct calls to the cold function.  (This is
inconsistent with other attributes such as const and pure which can
be applied to function pointers, so I wouldn't recommend relying on
cold not changing to match the others.)

Martin

PS For reference, the built-ins that GCC implicitly declares cold
are __builtin_abort, __builtin_trap and __builtin_unreachable.

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Ard Biesheuvel]

On Mon, 11 Feb 2019 at 17:21, Martin Sebor <msebor@gmail.com> wrote:
>
> On 2/9/19 5:31 AM, Miguel Ojeda wrote:
> > On Sat, Feb 9, 2019 at 12:26 PM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On Sat, 9 Feb 2019 at 12:19, Miguel Ojeda
> >> <miguel.ojeda.sandonis@gmail.com> wrote:
> >>>
> >>> It also affects the optimizer in two different ways AFAIK:
> >>>
> >>>    * For the function itself, it gets optimized for size instead of speed.
> >>>    * For callers, the paths that lead to the calls are treated as unlikely.
> >>>
> >>
> >> That seems reasonable, but that still does not mean it is necessarily
> >> a problem if you apply 'cold' to one but not the other.
> >
> > Indeed. As I said, it is likely that you missed the attribute, not a
> > sure thing (i.e. that you didn't do it explicitly).
> >
> >>> So GCC reports it because you would be (likely) missing the
> >>> optimizations you expected if you are using the alias instead of the
> >>> target.
> >>>
> >>
> >> I see how that could be reasonable for extern declarations that do not
> >> match the definition, since in that case, it is assumed that there is
> >> only one instance of the function. For function pointers, I don't
> >> think this assumption is valid.
> >
> > It sounds reasonable to have another warning for
> > declarations-definition attribute mismatches too. However, I don't see
> > why you would warn differently. Even if you have one instance of the
> > function, you may also be using the declaration to explicitly avoid
> > the unlikely treatment.
> >
> > Now, whether the warning is worth or not or at which "level", it
> > depends. I guess the rationale behind having it under -Wall is that
> > they expect people to have missed copying the attributes, rather than
> > they are using aliases specifically to avoid a cold/... attribute.
> >
> >>> In our case in patch 3, we do not want the optimization for callers,
> >>> which is why we don't mark the extern declaration as __cold (see the
> >>> commit message).
> >>>
> >>
> >> You did not cc me on the whole set, so I don't have the patch. But in
> >> any case, GCC 9 has not been released so we should still have time to
> >> talk sense into the GCC guys.
> >
> > I only CC'd people on the relevant patches according to
> > get_maintainers (but yeah, 2 & 3 are related, I could have merged
> > those lists). Anyway, using the lore.kernel.org server makes it easy
> > to see an entire series when you have already one:
> >
> >    https://lore.kernel.org/lkml/20190209000840.11018-1-miguel.ojeda.sandonis@gmail.com/
> >
> > As for GCC, Martin (the author of the features) is CC'd, so he can
> > chime in (and I am sure he appreciates the feedback :-)
>
> I do very much, thank you.  I think you've fielded all the GCC
> questions here so I don't have much to add.  Just a comment regarding
> aliases with different sets of attributes than their targets: it is
> possible to come up with use cases not just with attribute cold but
> others as well, including mutually exclusive pairs such as const and
> pure.  But the uses cases that drove the design of the warning were
> those where the set of attributes is meant to be the same, and we
> are yet to to come across the others in practice.  If it turns out
> that there are others in common use we can tweak the warning logic.
>
> Regarding function pointers, attribute cold only applies to function
> declarations (and labels) and is ignored on pointers, so calls to
> a cold function through a pointer are not necessarily subject to
> the same effects as direct calls to the cold function.  (This is
> inconsistent with other attributes such as const and pure which can
> be applied to function pointers, so I wouldn't recommend relying on
> cold not changing to match the others.)
>

Hello Martin,

Thanks for clearing this up. I seemed to have confused myself into
thinking that the slew of attribute 'cold' warnings I was seeing when
building the kernel plus modules with GCC-9 were about function
pointers, but in fact they were about symbol aliases, for which the
warning makes perfect sense.

Next time, I'll try talking some sense into myself first. Apologies
for the noise.

-- 
Ard.

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Laura Abbott]

On 2/9/19 12:08 AM, Miguel Ojeda wrote:
> The upcoming GCC 9 release extends the -Wmissing-attributes warnings
> (enabled by -Wall) to C and aliases: it warns when particular function
> attributes are missing in the aliases but not in their target, e.g.:
> 
>      void __cold f(void) {}
>      void __alias("f") g(void);
> 
> diagnoses:
> 
>      warning: 'g' specifies less restrictive attribute than
>      its target 'f': 'cold' [-Wmissing-attributes]
> 
> These patch series clean these new warnings. Most of them are caused
> by the module_init/exit macros.
> 
> The first patch has been in -next for a long time already, and an alternative
> solution (only __cold) for module.h as well. However, since we decided
> to go with the new __copy attribute, I will leave the series for a few days
> again and send the PR for -rc7.
> 
> Link: https://lore.kernel.org/lkml/20190125104353.2791-1-labbott@redhat.com/
> 
> Miguel Ojeda (3):
>    lib/crc32.c: mark crc32_le_base/__crc32c_le_base aliases as __pure
>    Compiler Attributes: add support for __copy (gcc >= 9)
>    include/linux/module.h: copy __init/__exit attrs to
>      init/cleanup_module
> 
>   include/linux/compiler_attributes.h | 14 ++++++++++++++
>   include/linux/module.h              |  4 ++--
>   lib/crc32.c                         |  4 ++--
>   3 files changed, 18 insertions(+), 4 deletions(-)
> 


For the entire series:

Tested-by: Laura Abbott <labbott@redhat.com>

You can look at the full build logs at
https://koji.fedoraproject.org/koji/taskinfo?taskID=326911 .

Thanks,
Laura

Re: [PATCH 0/3] Clean the new GCC 9 -Wmissing-attributes warnings

[Miguel Ojeda]

On Sat, Feb 9, 2019 at 9:33 PM Laura Abbott <labbott@redhat.com> wrote:
>
> Tested-by: Laura Abbott <labbott@redhat.com>
>
> You can look at the full build logs at
> https://koji.fedoraproject.org/koji/taskinfo?taskID=326911 .
>
> Thanks,
> Laura

Thanks a lot for the tests, Laura!

I will send this for -rc7.

Cheers,
Miguel