Re: [PATCH] c/c++: Tweak warning for 'always_inline function might not be inlinable'

[Richard Biener <richard.guenther@gmail.com>]

On Sun, Jan 21, 2024 at 2:33 PM Hans-Peter Nilsson <hp@axis.com> wrote:
>
> Tested x86_64-linux-gnu.  Ok to commit?
>
> Or, does the message need more tweaking?
> (If so, suggestions from native speakers?)
> FWIW, I found no PR for just the message being bad.
>
> -- >8 --
> When you're not regularly exposed to this warning, it is
> easy to be misled by its wording, believing that there's
> something else in the function that stops it from being
> inlined, than the lack of also being *declared* inline.
>
> It's just a warning: without the inline directive, there has
> to be a secondary reasons for the function to be inlined,
> than the always_inline attribute, reasons that may be in
> effect despite the warning.
>
> Whenever the text is quoted in inline-related bugzilla
> entries, there seems to often have been an initial step of
> confusion that has to be cleared, for example in PR55830.
> The powerpc test-case in this patch where a comment is
> adjusted, seems to be another example, and I testify as the
> first-hand third "experience".  The wording has been the
> same since the warning was added.
>
> Let's just tweak the wording, adding the cause, so that the
> reason for the warning is clearer.  This hopefully stops the
> user from immediately asking "'Might'?  Because why?"  and
> then going off looking at the function body - or grepping
> the gcc source or documentation, or enter a bug-report
> subsequently closed as resolved/invalid.
>
> gcc:
>         * cgraphunit.cc (process_function_and_variable_attributes): Tweak
>         the warning for an attribute-always_inline without inline declaration.
>
> gcc/testsuite:
>         * g++.dg/Wattributes-3.C: Adjust expected warning.
>         * gcc.dg/fail_always_inline.c: Ditto.
>         * gcc.target/powerpc/vec-extract-v16qiu-v2.h: Adjust
>         quoted warning in comment.
> ---
>  gcc/cgraphunit.cc                                        | 3 ++-
>  gcc/testsuite/g++.dg/Wattributes-3.C                     | 4 ++--
>  gcc/testsuite/gcc.dg/fail_always_inline.c                | 2 +-
>  gcc/testsuite/gcc.target/powerpc/vec-extract-v16qiu-v2.h | 2 +-
>  4 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/cgraphunit.cc b/gcc/cgraphunit.cc
> index 38052674aaa5..89dc1af522a4 100644
> --- a/gcc/cgraphunit.cc
> +++ b/gcc/cgraphunit.cc
> @@ -918,7 +918,8 @@ process_function_and_variable_attributes (cgraph_node *first,
>           /* redefining extern inline function makes it DECL_UNINLINABLE.  */
>           && !DECL_UNINLINABLE (decl))
>         warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
> -                   "%<always_inline%> function might not be inlinable");
> +                   "%<always_inline%> function is not always inlined"
> +                   " unless also declared %<inline%>");

I don't like the "is not always inlined", maybe simply reword to

  "%<always_inline%> function might not be inlinable"
  " unless also declared %<inline%>"

?

>
>        process_common_attributes (node, decl);
>      }
> diff --git a/gcc/testsuite/g++.dg/Wattributes-3.C b/gcc/testsuite/g++.dg/Wattributes-3.C
> index 208ec6696551..4adf0944cd4f 100644
> --- a/gcc/testsuite/g++.dg/Wattributes-3.C
> +++ b/gcc/testsuite/g++.dg/Wattributes-3.C
> @@ -26,7 +26,7 @@ B::operator char () const { return 0; }
>
>  ATTR ((__noinline__))
>  B::operator int () const      // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
> -// { dg-warning "function might not be inlinable" "" { target *-*-* } .-1 }
> +// { dg-warning "function is not always inlined unless also declared .inline." "" { target *-*-* } .-1 }
>
>  {
>    return 0;
> @@ -45,7 +45,7 @@ C::operator char () { return 0; }
>
>  ATTR ((__noinline__))
>  C::operator short ()           // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
> -// { dg-warning "function might not be inlinable" "" { target *-*-* } .-1 }
> +// { dg-warning "function is not always inlined unless also declared .inline." "" { target *-*-* } .-1 }
>  { return 0; }
>
>  inline ATTR ((__noinline__))
> diff --git a/gcc/testsuite/gcc.dg/fail_always_inline.c b/gcc/testsuite/gcc.dg/fail_always_inline.c
> index 86645b850de8..2f48d7f5c6be 100644
> --- a/gcc/testsuite/gcc.dg/fail_always_inline.c
> +++ b/gcc/testsuite/gcc.dg/fail_always_inline.c
> @@ -2,7 +2,7 @@
>  /* { dg-add-options bind_pic_locally } */
>
>  extern __attribute__ ((always_inline)) void
> - bar() { } /* { dg-warning "function might not be inlinable" } */
> + bar() { } /* { dg-warning "function is not always inlined unless also declared .inline." } */
>
>  void
>  f()
> diff --git a/gcc/testsuite/gcc.target/powerpc/vec-extract-v16qiu-v2.h b/gcc/testsuite/gcc.target/powerpc/vec-extract-v16qiu-v2.h
> index d1157599ee7c..b9800e1c950d 100644
> --- a/gcc/testsuite/gcc.target/powerpc/vec-extract-v16qiu-v2.h
> +++ b/gcc/testsuite/gcc.target/powerpc/vec-extract-v16qiu-v2.h
> @@ -179,7 +179,7 @@ get_auto_15 (vector TYPE a)
>  #ifdef DISABLE_INLINE_OF_GET_AUTO_N
>  __attribute__ ((__noinline__))
>  #else
> -/* gcc issues warning: always_inline function might not be inlinable
> +/* gcc issues warning: always_inline function is not always inlined unless also declared inline
>
>     __attribute__ ((__always_inline__))
>  */
> --
> 2.30.2
>