| Message ID | 20240121133239.45A0B20422@pchp3.se.axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | c/c++: Tweak warning for 'always_inline function might not be inlinable' | expand |
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 >
> 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
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
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__)) */