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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: jgilbert, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
(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.
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
(Reporter)

Comment 3

7 years ago
Created attachment 570109 [details] [diff] [review]
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.
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.
(Reporter)

Comment 5

7 years ago
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.
Duplicate of this bug: 798954

Comment 7

6 years ago
Jeff G., any progress on this?
Blocks: 187528

Comment 8

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

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Comment 9

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

Comment 10

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.