697810 - NS_ALWAYS_INLINE is misleading, being incomplete on GNUC, and inactive on other platforms

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[Kelsey Gilbert [:jgilbert]]

NS_ALWAYS_INLINE, despite appearing to be our standard keyword for attempting to force the inlining of a function, is actually only a define set by the availability of GNUC's '__attribute__((always_inline))'. Further more, to get the proper effect, '__attribute__((always_inline))' is not sufficient by itself, and should actually be '__attribute__((always_inline)) inline'.

In mozalloc.h, the helper for forcing inlining is:

#if defined(NS_ALWAYS_INLINE)
#  define MOZALLOC_INLINE NS_ALWAYS_INLINE inline
#elif defined(HAVE_FORCEINLINE)
#  define MOZALLOC_INLINE __forceinline
#else
#  define MOZALLOC_INLINE inline
#endif


A more sensible name for 'NS_ALWAYS_INLINE' would instead be similar to 'HAVE_FORCEINLINE' for MSVC's __forceinline keyword, such as 'HAVE_GNUC_ALWAYS_INLINE'. From there, we can define a NS_ALWAYS_INLINE keyword to try to force inlining on all platforms:

#if defined(HAVE_GNUC_ALWAYS_INLINE)
#  define NS_ALWAYS_INLINE __attribute__((always_inline)) inline
#elif defined(HAVE_FORCEINLINE)
#  define NS_ALWAYS_INLINE __forceinline
#else
#  define NS_ALWAYS_INLINE inline
#endif

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #0)
> NS_ALWAYS_INLINE, despite appearing to be our standard keyword for
> attempting to force the inlining of a function, is actually only a define
> set by the availability of GNUC's '__attribute__((always_inline))'. Further
> more, to get the proper effect, '__attribute__((always_inline))' is not
> sufficient by itself, and should actually be '__attribute__((always_inline))
> inline'.

Agree; the reason for '__attribute__((always_inline)) inline' is that __attribute__((always_inline)) by itself only forces inlining but doesn't allow multiple definitions (across multiple translation units) like 'inline' does.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Also, note that we could completely remove the configure checks for __attribute__((always_inline)) and __forceinline and instead do as in bug 697450:

#if defined _MSC_VER
#define FORCE_INLINE __forceinline
#elif defined __GNUC__
#define FORCE_INLINE __attribute__((always_inline)) inline
#else
#define FORCE_INLINE inline
#endif

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: WIP fix which allows NS_ALWAYS_INLINE to be used to attempt to force inlining

Here is an example that only slightly refactors the configure.in implementation, as per my earlier comment.

Implementation is split into three places, currently. 'nscore.h', 'mozalloc.h', and another copy in libjpeg. 'nscore.h' and 'mozalloc.h' copies should be consolidated, but I'm not sure about how to best go about this. libjpeg copy should probably stay separate, but I'm not positive about that either.

Comment 4

[Jeff Walden [:Waldo]]

This looks like a lovely little bikeshed.  Not meaning to throw a new coat of paint on it, but perhaps the best solution would be to remove all this NS_*INLINE stuff and just use MOZ_INLINE and MOZ_ALWAYS_INLINE in mfbt/Util.h instead?  They seem to not have the attribute-but-not-inline problem which triggered this.  Also, mfbt/ is the new better-curated thing intended to be used by everyone, avoiding further proliferation problems.

Comment 5

[Kelsey Gilbert [:jgilbert]]

MOZ_ALWAYS_INLINE is perfect; I was unaware of it. I'll throw a patch together to drop the old stuff in favor of the MOZ_ analogues.

Comment 7

[:aceman]

Jeff G., any progress on this?

Comment 8

[(no longer active)]

I don't think this is worth fixing.  We should just fix bug 800106.

Comment 9

[:aceman]

I think this was the plan here (comment 5), so why a new bug? Or mark a dupe?

Comment 10

[(no longer active)]

(In reply to comment #9)
> I think this was the plan here (comment 5), so why a new bug? Or mark a dupe?

Because bug 800106 is a mentored bug, and the comments here doesn't make it very easy for a new contributor to pick up that bug.