Closed Bug 697810 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jgilbert, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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.
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
Jeff G., any progress on this?
Blocks: buildwarning
I don't think this is worth fixing.  We should just fix bug 800106.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
I think this was the plan here (comment 5), so why a new bug? Or mark a dupe?
(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.