964852 - Assertions.h:139:1: warning: always_inline function might not be inlinable [-Wattributes]

Status: RESOLVED WORKSFORME

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

I get a fair number of these "always_inline function might not be inlinable" warnings when compiling.  The always_inline functions in question are the functions for reporting assertion errors, which shouldn't need to be inlined.  (In fact, I'd be a little surprised if the compiler inlined them, since they're calling varargs functions, which are bound to be a little more expensive than calling fixed-arg functions.)

We should simply declare them extern and move the implementations someplace else.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: move definitions of MOZ_Report* out-of-line

Just like the patch says.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8366854 [details] [diff] [review]
move definitions of MOZ_Report* out-of-line

Review of attachment 8366854 [details] [diff] [review]:
-----------------------------------------------------------------

Any reason not to just s/MOZ_ALWAYS_INLINE/inline/g and leave it at that?  Don't see much point in out-of-lining effort here to avoid an easy warning.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Any reason not to just s/MOZ_ALWAYS_INLINE/inline/g and leave it at that? 
> Don't see much point in out-of-lining effort here to avoid an easy warning.

Hm, no, other than #include'ing slightly less stuff and getting a slightly smaller binary.  I guess I'll do that instead, assuming it builds everywhere.

Comment 4

[Nathan Froyd [:froydnj]]

(Out-of-lining saves ~0.2% of text size on x86-64 debug and a whopping 250 bytes on x86-64 opt, FWIW.)

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> > Any reason not to just s/MOZ_ALWAYS_INLINE/inline/g and leave it at that? 
> > Don't see much point in out-of-lining effort here to avoid an easy warning.
> 
> Hm, no, other than #include'ing slightly less stuff and getting a slightly
> smaller binary.  I guess I'll do that instead, assuming it builds everywhere.

It does not build, particularly on Windows.  We use Assertions.h in C source files (jemalloc, for instance), and MSVC doesn't recognize |inline| in C files.  Hooray.

WDYT about adding MOZ_INLINE with strong warnings that you shouldn't be using it if you know your code will only be used from C++?

Comment 6

[Masatoshi Kimura [:emk]]

FYI I removed MOZ_INLINE in bug 928210.

Comment 7

[Nathan Froyd [:froydnj]]

I suppose another option is to simply #ifdef __cplusplus the |inline| bit for those functions with an explanatory comment somewhere.

Comment 8

[Jeff Walden [:Waldo]]

#ifdef _MSC_VER
__inline
#else
inline
#endif

seems slightly preferable to reintroducing MOZ_INLINE for not too much value.  (Particularly if, as I hope, we eventually convert to building everything in the tree as C++ and not as C, to simplify our language compatibility matrix and all.)

Comment 9

[Chris Peterson [:cpeterson]]

btw, just yesterday I wrote a patch proposing to `#define inline __inline` when compiling C code with MSVC (to fix some warnings in webrtc's C code). Redefining a keyword like `inline` is reckless, but it is unreasonable for a modern C compiler to not support understand `inline`.

+#if defined(_MSC_VER) && !defined(__cplusplus) && !defined(inline)
+#  define inline                __inline
+#endif
+
 #if !defined(DEBUG)
 #  define MOZ_ALWAYS_INLINE     MOZ_ALWAYS_INLINE_EVEN_DEBUG
-#elif defined(_MSC_VER) && !defined(__cplusplus)
-#  define MOZ_ALWAYS_INLINE     __inline
 #else
 #  define MOZ_ALWAYS_INLINE     inline
 #endif

Comment 10

[Masatoshi Kimura [:emk]]

MSVC12 dislikes #defining the "inline" keyword.

Comment 11

[Chris Peterson [:cpeterson]]

Nathan, do you still see these -Wattributes warnings about always_inline? I don't see them when build for Android with gcc 4.9. (I don't have a Linux build environment.)

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Nathan, do you still see these -Wattributes warnings about always_inline? I
> don't see them when build for Android with gcc 4.9. (I don't have a Linux
> build environment.)

I don't see them on my local 4.9 build.  I guess 4.9 changed the warning somehow?  (I'm pretty sure I was using 4.7 or 4.8 previously...)

Comment 13

[Chris Peterson [:cpeterson]]

Thanks. I'm triaging old bug reports about compiler warnings. I'll close this bug unless someone sees this warning again.