340244 - Make NS_LIKELY/NS_UNLIKELY accept pointers on all platforms

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Mats Palmgren (inactive)]

The following fails to compile on at least one platform:

if (NS_UNLIKELY(frame));  // where 'frame' is a nsIFrame*

This is the error I got on the "Linux prometheus" Tinderbox:
nsLineBox.cpp:518: error: invalid conversion from `nsIFrame*' to `long int'

A bit of info from "prometheus" build log:
uname: Linux prometheus.mozilla.org 2.4.21-27.0.4.ELsmp #1 SMP Sat Apr 16
18:43:06 EDT 2005 i686 i686 i386 GNU/Linux
Compiler is -- gcc (gcc (GCC) 3.3.2 20031022 (Red Hat Linux 3.3.2-1)


My opinion is that if "if(x);" compiles then so should "if(NS_UNLIKELY(x));" for any given x.

Comment 1

[Brian Ryner (not reading)]

Does changing the macro definition to this fix it?

#define NS_UNLIKELY(x)  (__builtin_expect((x != 0), 0))

Comment 2

[Mats Palmgren (inactive)]

I would think so (if you wrap the x in parens), I haven't got access to "prometheus" so I can't test... Here's an alternative that I think would
work as well:
#define NS_UNLIKELY(x)  (__builtin_expect(!!(x), 0))

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Tested with gcc 4.1.2 on Linux/x86_64 and gcc 3.3.1 on Linux/i386

Comment 4

[Mats Palmgren (inactive)]

I checked this in today but had to back it out because it made SeaMonkey
Tinderboxes "Linux lhasa Dep release (gtk2+xft)" and "Linux luna Dep"
orange:

=======================================================================
...
seamonkey-bin built successfully.
seamonkey-bin binary exists, build successful.


Running regxpcom test ...
Timeout = 120 seconds.
Begin: Thu Nov  9 11:04:11 2006
cmd = /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/mozilla//dist/bin/regxpcom
End:   Thu Nov  9 11:04:17 2006
----------- Output from regxpcom ------------- 
----------- End Output from regxpcom --------- 
regxpcom: test failed
Found profile.
Creating clean profile ...
Deleting /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/.mozilla/ ...
Begin: Thu Nov  9 11:04:17 2006
cmd = /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/mozilla//dist/bin/seamonkey-bin -CreateProfile default
End:   Thu Nov  9 11:04:21 2006
----------- Output from Profile Creation ------------- 
  
  (Gecko:7218): GLib-WARNING **: Priorities can only be increased by root.
----------- End Output from Profile Creation --------- 
ERROR: couldn't find prefs.js in /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/.mozilla/
no pref file found
=======================================================================

After backing it out the Tinderboxes went green again so it was
this patch that caused the problem although I can't understand why
right now...  (there was no problem on Firefox or Camino Tinderboxes
that runs the same test.)

Comment 5

[Mats Palmgren (inactive)]

__builtin_expect returns its first arg (type long):
http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005fexpect-2455

NS_(UN)LIKELY returns what __builtin_expect returns.
NS_FAILED returns what NS_UNLIKELY returns:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nsError.h&rev=1.49&root=/cvsroot&mark=113#112

The patch gives a different result when assigning the NS_FAILED(rv) result
to a PRPackedBool (= PRUint8), the Tinderbox orange was caused by:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libpref/src/nsPrefService.cpp&rev=1.91&root=/cvsroot&mark=375#358

(The error probably occurs for all assignments to a variable of
integral type narrower than 'long'.)

This test:
    PRBool       old1 = __builtin_expect((0x80000000), 0);
    PRPackedBool old2 = __builtin_expect((0x80000000), 0);
    PRBool       new1 = __builtin_expect(!!(0x80000000), 0);
    PRPackedBool new2 = __builtin_expect(!!(0x80000000), 0);
    printf("old(%d %d) new(%d %d)\n", old1, old2, new1, new2);

prints: old(-2147483648 0) new(1 1)

I think the current definition of NS_FAILED is to blame.
Shouldn't it return 0 or 1, like NS_SUCCEEDED?

The nsPrefService code seems to depend on this bug though...

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Shouldn't it return 0 or 1, like NS_SUCCEEDED?

In my opinion, yes!

> The nsPrefService code seems to depend on this bug though...

The mErrorOpeningUserPrefs thing?  You mean the fact that it's always 0 currently?

Seems to me like that's just a bug we should fix, no?  Or just nix that member, if it's not useful anyway.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 245049 [details] [diff] [review]
Patch rev. 1

You should really fix the patch to put the !! in the #else-half as well, so bugs like this show up in all builds rather than just some.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

1. add !! to the #else part
2. remove the always-false flags in nsPrefService -
   I will file a follow-up bug to restore them because I think
   not overwriting an existing prefs.js when there are read errors
   seems like a good idea (assuming this was the intention).
3. fixed some dubious tests in some DEBUG-only editor code
   that assumes the value of NS_FAILED(rv) == NS_OK when
   'rv' is the result of a successful operation
4. I added comments to NS_FAILED/SUCCEEDED and NS_(UN)LIKELY 
   making it clear that they are guaranteed to return 0 or 1 now

I tested this on several platforms. I also compiled a version
where I made these macros return a class object with a private
'operator char' to catch all assignments to PR(Packed)Bool (for the
configuration I built at least) and nsPrefService was the only error.
(I found a few 'PRPackedBool initialized = NS_SUCCEEDED(rv)' but
those should be ok.)

BTW, I think the recommended usage example in the comment encourages
code that doesn't follow the coding rules:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nscore.h&rev=1.98&root=/cvsroot&mark=451-457#447

Wouldn't the following be better?

 *  if (NS_LIKELY(v)) {
 *    ... expected code path ...
 *  }
 *
 *  if (NS_UNLIKELY(!v)) {
 *    ... non-expected code path ...
 *  }

which covers the following common cases:
  if (NS_LIKELY(pointer)) { ...
  if (NS_UNLIKELY(!pointer)) { ...
which is preferred over
  if (NS_LIKELY(pointer != nsnull)) { ...
  if (NS_UNLIKELY(pointer == nsnull)) { ...

Comment 9

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-11-17 16:48 PST

Filed bug 361102 on fixing point 2. above

-> FIXED