Re: [boost] [Boost-users] [signals2][variant] inline warning with MSVC 2022
On Wed, Sep 21, 2022 at 12:21 PM Frank Mori Hess <fmh6jj@gmail.com> wrote:
On Mon, Sep 19, 2022 at 3:16 AM Gavin Lambert via Boost-users <boost-users@lists.boost.org> wrote:
,struct boost::mpl::l_iter<struct boost::mpl::l_end> > *)' marked as __forceinline not inlined
I'm not sure whether the Real Bug™ is in signals2 or in variant, but I don't think it's in any of my code since it doesn't name any of my types above that I can see.
The problem is not in signals2, as it makes no direct use of __forceinline. variant does make use of BOOST_FORCEINLINE so it probably needs to have the pragma suppression added somewhere.
Actually, maybe the easiest thing would be to just put the pragma suppression in the header that defines BOOST_FORCEINLINE (config/detail/suffix.hpp). -- Frank
Frank Mori Hess wrote:
On Wed, Sep 21, 2022 at 12:21 PM Frank Mori Hess <fmh6jj@gmail.com> wrote:
On Mon, Sep 19, 2022 at 3:16 AM Gavin Lambert via Boost-users <boost-users@lists.boost.org> wrote:
,struct boost::mpl::l_iter<struct boost::mpl::l_end> > *)' marked as __forceinline not inlined
I'm not sure whether the Real Bug™ is in signals2 or in variant, but I don't think it's in any of my code since it doesn't name any of my types above that I can see.
The problem is not in signals2, as it makes no direct use of __forceinline. variant does make use of BOOST_FORCEINLINE so it probably needs to have the pragma suppression added somewhere.
Actually, maybe the easiest thing would be to just put the pragma suppression in the header that defines BOOST_FORCEINLINE (config/detail/suffix.hpp).
That's not very user friendly; maybe the user _wants_ the warning in his code, and the above would disable it even if no Boost header that uses BOOST_FORCEINLINE (and generates a warning) has been included.
On Wed, Sep 21, 2022 at 12:32 PM Peter Dimov <pdimov@gmail.com> wrote:
That's not very user friendly; maybe the user _wants_ the warning in his code, and the above would disable it even if no Boost header that uses BOOST_FORCEINLINE (and generates a warning) has been included.
Okay, then maybe BOOST_FORCEINLINE shouldn't be defined as __force_inline on compilers where it doesn't actually force inlining. If the compiler is just going to treat it as a suggestion, I don't see the point of boost headers using it and then suppressing the warning. -- Frank
Am 22.09.22 um 06:24 schrieb Frank Mori Hess via Boost:
That's not very user friendly; maybe the user _wants_ the warning in his code, and the above would disable it even if no Boost header that uses BOOST_FORCEINLINE (and generates a warning) has been included. Okay, then maybe BOOST_FORCEINLINE shouldn't be defined as __force_inline on compilers where it doesn't actually force inlining. If the compiler is just going to treat it as a suggestion, I don't see
On Wed, Sep 21, 2022 at 12:32 PM Peter Dimov <pdimov@gmail.com> wrote: the point of boost headers using it and then suppressing the warning.
I'd rather see the headers generating this warning also suppress them. Yes it may not actually "force" inlining but IIRC it serves as a strong hint to the compiler, so removing it is not a good choice.
Frank Mori Hess wrote:
On Wed, Sep 21, 2022 at 12:32 PM Peter Dimov <pdimov@gmail.com> wrote:
That's not very user friendly; maybe the user _wants_ the warning in his code, and the above would disable it even if no Boost header that uses BOOST_FORCEINLINE (and generates a warning) has been included.
Okay, then maybe BOOST_FORCEINLINE shouldn't be defined as __force_inline on compilers where it doesn't actually force inlining.
That's complete nonsense. First, Microsoft invented __forceinline, so making BOOST_FORCEINLINE be a no-op on the very compiler for which the feature has been added would be ridiculous. Second, it does force inlining, that's why it's called __forceinline. The whole point of the warning is to warn about the rare cases where the compiler does not honor the command; it exists because people want to know whether their "force inline" orders are ignored.
On 9/22/22 15:57, Peter Dimov via Boost wrote:
Frank Mori Hess wrote:
On Wed, Sep 21, 2022 at 12:32 PM Peter Dimov <pdimov@gmail.com> wrote:
That's not very user friendly; maybe the user _wants_ the warning in his code, and the above would disable it even if no Boost header that uses BOOST_FORCEINLINE (and generates a warning) has been included.
Okay, then maybe BOOST_FORCEINLINE shouldn't be defined as __force_inline on compilers where it doesn't actually force inlining.
That's complete nonsense. First, Microsoft invented __forceinline, so making BOOST_FORCEINLINE be a no-op on the very compiler for which the feature has been added would be ridiculous. Second, it does force inlining, that's why it's called __forceinline. The whole point of the warning is to warn about the rare cases where the compiler does not honor the command; it exists because people want to know whether their "force inline" orders are ignored.
I should add that I don't think there is *any* compiler that has a way to 100% guarantee that a function will be inlined. So BOOST_FORCEINLINE is a strong, but nonetheless a hint, on every compiler that provides a suitable function attribute.
On Thu, Sep 22, 2022 at 8:57 AM Peter Dimov <pdimov@gmail.com> wrote:
That's complete nonsense. First, Microsoft invented __forceinline, so making BOOST_FORCEINLINE be a no-op on the very compiler for which the feature has been added would be ridiculous. Second, it does force inlining, that's why it's called __forceinline. The whole point of the warning is to warn about the rare cases where the compiler does not honor the command; it exists because people want to know whether their "force inline" orders are ignored.
But no boost user wants to be warned about some internal implementation detail of a boost library. So every boost usage of BOOST_FORCEINLINE needs to add a #pragma warning(disable: 4714), awesome! Who is the maintainer of variant anyways? -- Frank
Frank Mori Hess wrote:
On Thu, Sep 22, 2022 at 8:57 AM Peter Dimov <pdimov@gmail.com> wrote:
That's complete nonsense. First, Microsoft invented __forceinline, so making BOOST_FORCEINLINE be a no-op on the very compiler for which the feature has been added would be ridiculous. Second, it does force inlining, that's why it's called __forceinline. The whole point of the warning is to warn about the rare cases where the compiler does not honor the command; it exists because people want to know whether their "force inline" orders are ignored.
But no boost user wants to be warned about some internal implementation detail of a boost library.
Yes of course. But users do want to be warned about their own code. And if Boost.Config disables the warning, they won't be, even if they don't use variant at all.
Who is the maintainer of variant anyways?
Antony Polukhin, I believe. https://github.com/boostorg/boost/blob/80e75653fcd6b9ad145f9a103d5c5a592a221... Note by the way that MSVC honors the #pragma warning even if it's applied locally around the __forceinline function: https://godbolt.org/z/sW99TYa66 So the variant header can do this, without affecting other uses. Is there an issue in boostorg/variant about this problem?
On 9/23/22 00:31, Peter Dimov via Boost wrote:
Frank Mori Hess wrote:
On Thu, Sep 22, 2022 at 8:57 AM Peter Dimov <pdimov@gmail.com> wrote:
That's complete nonsense. First, Microsoft invented __forceinline, so making BOOST_FORCEINLINE be a no-op on the very compiler for which the feature has been added would be ridiculous. Second, it does force inlining, that's why it's called __forceinline. The whole point of the warning is to warn about the rare cases where the compiler does not honor the command; it exists because people want to know whether their "force inline" orders are ignored.
But no boost user wants to be warned about some internal implementation detail of a boost library.
Yes of course. But users do want to be warned about their own code. And if Boost.Config disables the warning, they won't be, even if they don't use variant at all.
Maybe we could use __pragma in BOOST_FORCEINLINE on a recent enough MSVC? https://godbolt.org/z/oh5szGd4r
Am 23.09.22 um 09:18 schrieb Andrey Semashev via Boost:
Maybe we could use __pragma in BOOST_FORCEINLINE on a recent enough MSVC?
https://godbolt.org/z/oh5szGd4r This is an awesome idea and I'm surprised it actually works.
I wanted to propose a PR to Boost.Config which adds your code to the test suite making sure it compiles warning-free. However it fails on GCC:
error: inlining failed in call to ‘always_inline’ ‘unsigned int f_not_inlined(unsigned int)’: function not considered for inlining 19 | BOOST_FORCEINLINE unsigned f_not_inlined( unsigned x ) | ^~~~~~~~~~~~~ note: called from here 21 | return x == 0 ? 0: x * f_not_inlined( x-1 );
So I'm actually not sure how we want to define the behavior of BOOST_FORCEINLINE especially as it is a macro officially available for users too. Given the name a warning is appropriate especially given that other compilers error in such cases. For the GCC behavior I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63220 which cites
always_inline is _not_ an optimization hint! always_inline was meant to mark functions that won't work correctly if not inlined.
So I'd say we need a `BOOST_INLINE` macro expanding to a (strong) inlining hint and does not generate errors/warnings. And Boost libraries should most likely use that, not the FORCEINLINE macro.
On 9/23/22 11:25, Alexander Grund via Boost wrote:
Am 23.09.22 um 09:18 schrieb Andrey Semashev via Boost:
Maybe we could use __pragma in BOOST_FORCEINLINE on a recent enough MSVC?
https://godbolt.org/z/oh5szGd4r This is an awesome idea and I'm surprised it actually works.
I wanted to propose a PR to Boost.Config which adds your code to the test suite making sure it compiles warning-free.
However it fails on GCC:
error: inlining failed in call to ‘always_inline’ ‘unsigned int f_not_inlined(unsigned int)’: function not considered for inlining 19 | BOOST_FORCEINLINE unsigned f_not_inlined( unsigned x ) | ^~~~~~~~~~~~~ note: called from here 21 | return x == 0 ? 0: x * f_not_inlined( x-1 );
So I'm actually not sure how we want to define the behavior of BOOST_FORCEINLINE especially as it is a macro officially available for users too.
Given the name a warning is appropriate especially given that other compilers error in such cases. For the GCC behavior I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63220 which cites
always_inline is _not_ an optimization hint! always_inline was meant to mark functions that won't work correctly if not inlined.
So I'd say we need a `BOOST_INLINE` macro expanding to a (strong) inlining hint and does not generate errors/warnings. And Boost libraries should most likely use that, not the FORCEINLINE macro.
There's no middle ground behavior between inline keyword and BOOST_FORCEINLINE, so I don't see the reason to have yet another BOOST_INLINE macro. You want opportunistic inlining - use plain inline. If __pragma/_Pragma approach doesn't work, I'm inclined to shift the burden to library maintainers (those who are willing to deal with this) and users. To users: you've brought this on yourselves by using -Werror and -Wextra (or whatever equivalents your compiler has), so you pay the price by having to deal with this. Using max warnings, especially if promoted to errors, is useless and a bad idea. That said, I'm still in favor of an MSVC-specific fix by adding __pragmas to BOOST_FORCEINLINE. That pesky warning has come up a few times in the libraries I maintain and I never found it useful.
Am 23.09.22 um 11:59 schrieb Andrey Semashev via Boost:
There's no middle ground behavior between inline keyword and BOOST_FORCEINLINE, so I don't see the reason to have yet another BOOST_INLINE macro. You want opportunistic inlining - use plain inline.
If __pragma/_Pragma approach doesn't work, I'm inclined to shift the burden to library maintainers (those who are willing to deal with this) and users. To users: you've brought this on yourselves by using -Werror and -Wextra (or whatever equivalents your compiler has), so you pay the price by having to deal with this. Using max warnings, especially if promoted to errors, is useless and a bad idea.
That said, I'm still in favor of an MSVC-specific fix by adding __pragmas to BOOST_FORCEINLINE. That pesky warning has come up a few times in the libraries I maintain and I never found it useful.
How can you say that there is no middle-ground between inline and BOOST_FORCEINLINE but still want warnings disabled by default when the compiler doesn't do what the macro name suggests it is told to? The MSVC variant to what BOOST_FORCEINLINE expands to means inlining even with debug builds. I think that is useful. So sometimes you don't want to "force" inlining as the code works without it, but you want to strongly encourage the compiler to do so, which is what the MSVC attribute does. Hence the idea of a macro which does exactly that and also suppresses the warning when the function isn't inlined. Currently MSVC is the only compiler to provide something stronger than "inline" but other compilers may follow. And Boost.Config is the right place for expanding something like BOOST_INLINE to the MSVC attribute or "inline" when that is not supported. This is what similar macros also do.
On 9/23/22 14:06, Alexander Grund via Boost wrote:
Am 23.09.22 um 11:59 schrieb Andrey Semashev via Boost:
There's no middle ground behavior between inline keyword and BOOST_FORCEINLINE, so I don't see the reason to have yet another BOOST_INLINE macro. You want opportunistic inlining - use plain inline.
If __pragma/_Pragma approach doesn't work, I'm inclined to shift the burden to library maintainers (those who are willing to deal with this) and users. To users: you've brought this on yourselves by using -Werror and -Wextra (or whatever equivalents your compiler has), so you pay the price by having to deal with this. Using max warnings, especially if promoted to errors, is useless and a bad idea.
That said, I'm still in favor of an MSVC-specific fix by adding __pragmas to BOOST_FORCEINLINE. That pesky warning has come up a few times in the libraries I maintain and I never found it useful.
How can you say that there is no middle-ground between inline and BOOST_FORCEINLINE but still want warnings disabled by default when the compiler doesn't do what the macro name suggests it is told to?
The MSVC variant to what BOOST_FORCEINLINE expands to means inlining even with debug builds.
__forceinline does not work in debug builds. https://godbolt.org/z/v3srxv3sr OTOH, gcc's always_inline does. https://godbolt.org/z/97rMTKv9r As I said, BOOST_FORCEINLINE is not a guarantee that the function will be inlined. It is used as an optional hint. The fact that a certain call was not inlined is not an error, meaning that it is an expected possibility, however unfortunate it is. Which means that the warning is informational at best, and completely useless most of the time. I have not had a case when I wanted to see it, nor have I heard of someone wanting it, therefore I want to disable it. Now, I see that gcc's always_inline is intended to be stronger than that. However, in practice it showed to be an acceptable alternative to __forceinline - BOOST_FORCEINLINE has been defined like that for decades, and I don't remember anyone having a problem with that. If anything, there could be a need for a new macro that triggers an error if inlining fails, which is exactly what gcc's always_inline gives. But so far there were no requests for such a macro.
__forceinline does not work in debug builds.
https://godbolt.org/z/v3srxv3sr
OTOH, gcc's always_inline does.
https://godbolt.org/z/97rMTKv9r
As I said, BOOST_FORCEINLINE is not a guarantee that the function will be inlined. It is used as an optional hint. The fact that a certain call was not inlined is not an error, meaning that it is an expected possibility, however unfortunate it is. Which means that the warning is informational at best, and completely useless most of the time. I have not had a case when I wanted to see it, nor have I heard of someone wanting it, therefore I want to disable it.
Now, I see that gcc's always_inline is intended to be stronger than that. However, in practice it showed to be an acceptable alternative to __forceinline - BOOST_FORCEINLINE has been defined like that for decades, and I don't remember anyone having a problem with that.
If anything, there could be a need for a new macro that triggers an error if inlining fails, which is exactly what gcc's always_inline gives. But so far there were no requests for such a macro.
I think that goes too far IMO. Look, I actually think the status quo is correct, we have: * not declared inline, but might still be inlined if the compiler thinks it worth while * declared inline, should probably be inlined but might not be if something has to give. * delared __forceinline, a strong hint that if something has to give in the inlining, it should not be this. I actually think it's up to individual library authors, based on the use context, to decide whether they want to suppress warnings about a forced inline failing or not. Let me give you a concrete example: I recently reworked the Boost.Math complete elliptic integrals based on a paper which claimed to obtain equivalent performance to say std::log from their method. Now that's a big bold claim, to be able to do as good as something that's often a compiler intrinsic. Well it turns out that you can do that, but only if the function is expanded inline, and that only happens if the error handling is very slimline with no branch conditions (a "switch" was OK though). The result was that we could more or less match the claims made in the paper, with error handling (where the original paper had none), but only with careful coding and use of BOOST_FORCE_INLINE. In this case I believe I would want to see warnings about inlining not happening, because that would break our performance promises. BTW the difference between inlining or not is about an order of magnitude, and if you're making gazillions of calls to calculate the motion of your spacecraft ;) that could be important. I do appreciate the obnoxiousness of some of these warnings, but also remember that each new compiler release seems to introduce new warnings, so keeping Boost code warning free is next to impossible IMO (though no less desirable). John.
participants (5)
-
Alexander Grund -
Andrey Semashev -
Frank Mori Hess -
John Maddock -
Peter Dimov