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

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

[Hans-Peter Nilsson]

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%>");
 
       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

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

[Richard Biener]

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
>

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

[Hans-Peter Nilsson]

> From: Richard Biener <richard.guenther@gmail.com>
> Date: Mon, 22 Jan 2024 08:33:47 +0100

> > -                   "%<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%>"
> 
> ?

Sure.  Though it's a small nuance to which I don't actually
agree, I'll go along with almost anything as long as the
"<unless>...declared inline" augmentation is there :-)

Also, I can see that keeping closer to the original wording
as you suggest can be preferable to some.

I assume by your reply that the patch is ok with that change
but will wait another 72 hours for "native speakers" to have
a say.

Thanks!

brgds, H-P

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

[Richard Biener]

On Mon, Jan 22, 2024 at 3:27 PM Hans-Peter Nilsson <hp@axis.com> wrote:
>
> > From: Richard Biener <richard.guenther@gmail.com>
> > Date: Mon, 22 Jan 2024 08:33:47 +0100
>
> > > -                   "%<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%>"
> >
> > ?
>
> Sure.  Though it's a small nuance to which I don't actually
> agree, I'll go along with almost anything as long as the
> "<unless>...declared inline" augmentation is there :-)
>
> Also, I can see that keeping closer to the original wording
> as you suggest can be preferable to some.
>
> I assume by your reply that the patch is ok with that change
> but will wait another 72 hours for "native speakers" to have
> a say.

Yeah, it's OK with me with that change.  I CCed Honza in case
he has anything to add on the factual side.

Richard.

>
> Thanks!
>
> brgds, H-P

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

[Hans-Peter Nilsson]

Change from v1: The message is changed as per the review.
The powerpc test-case is dropped from the changes as the
part quoted in a comment now does not change and so cannot
cause further confusion.  The commit message is tweaked.
It now also mentions clang.  I intend to commit this on
Thursday 2024-01-25, as per Richard Biener's approval.

-- >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, something other than the lack of also being
*declared* inline.  Also, clang does not warn.

It's just a warning: without the inline directive, there has
to be a secondary reason for the function to be inlined,
other than the always_inline attribute, a reason 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.
A file in the powerpc-specific parts of the test-suite,
gcc.target/powerpc/vec-extract-v16qiu-v2.h, has a comment
and 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.

Since the message is only appended with additional
information, no test-case actually required adjustment.
I still changed them, so the message is covered.

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/cgraphunit.cc                         | 3 ++-
 gcc/testsuite/g++.dg/Wattributes-3.C      | 4 ++--
 gcc/testsuite/gcc.dg/fail_always_inline.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gcc/cgraphunit.cc b/gcc/cgraphunit.cc
index 38052674aaa5..5c405258ec30 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 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..dd9c2244900c 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 might not be inlinable 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 might not be inlinable 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..16a549ca0935 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 might not be inlinable unless also declared .inline." } */
 
 void
 f()
-- 
2.30.2