Closed Bug 964852 Opened 6 years ago Closed 4 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox46 --- verified

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Just like the patch says.
Attachment #8366854 - Flags: review?(jwalden+bmo)
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.
Attachment #8366854 - Flags: review?(jwalden+bmo) → review+
(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.
(Out-of-lining saves ~0.2% of text size on x86-64 debug and a whopping 250 bytes on x86-64 opt, FWIW.)
(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++?
Flags: needinfo?(jwalden+bmo)
FYI I removed MOZ_INLINE in bug 928210.
I suppose another option is to simply #ifdef __cplusplus the |inline| bit for those functions with an explanatory comment somewhere.
#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.)
Flags: needinfo?(jwalden+bmo)
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
MSVC12 dislikes #defining the "inline" keyword.
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.)
Flags: needinfo?(nfroyd)
(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...)
Flags: needinfo?(nfroyd)
Thanks. I'm triaging old bug reports about compiler warnings. I'll close this bug unless someone sees this warning again.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.