720350 - PRBool to bool problems for Firefox 10+

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Matej Spiller-Muys]

boolean type inside generated header files was changed from PRBool to bool.
Now the existing xpcom does not compile with new SDK.

Problem is that the size of bool is 1 byte (with win32 build of FF 10), when PRBool size is 4 bytes.

Essentialy every xpcom wrapper not using C is broken now (python, delphi, ...). And every xpcom is not compilable anymore (unless changed PRBool to bool).

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe this was a purposeful change.  The bug that made the change has a script that can be used to convert existing code to use bool instead of PRBool, right?

Comment 2

[Matej Spiller-Muys]

I was looking for the reason why this change was done and all i could find was: Let's switch from PRBool to bool.

This change means, that the size of bool is now compiler depended. Visual Studio versions uses 1 byte (some very old don't). Even firefox defines bool as 4 bytes if the target compiler does not define bool type. Internally using bool is fine, but when interoping (like xpcom) it's not. That's why MS uses VARIANT_BOOL type for automation where the size is fixed to 4.

This will introduce another layer of incompatibilities between different systems and compilers when creating xpcom. We will have to be very careful what compiler should we use when compiling. Some even redefine their own bool as int inside their c++ projects.

IDL should by definition have stable interfaces and not compiler dependend. The minumum should be that bool type should be checked inside that the size is 1.

GCC even has "one-byte-bool" flag, because default gcc bool size on mac is 4 bytes.

Somebody missed their own https://developer.mozilla.org/en/C++_Portability_Guide ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

The change was done because use of PRBool was actually very bug-prone, not to mention creating an additional bar to new contributors and causing performance issues in some cases.

> The minumum should be that bool type should be checked inside that the size is 1.

How would that help the issues you describe?  If it would, it would be worth doing.

> GCC even has "one-byte-bool" flag, because default gcc bool size on mac is 4 bytes.

Except it's not:

  #include <stdio.h>
  int main() {
    printf("Size of bool: %lu\n", sizeof(bool));
  }

  ~% uname -rs
  Darwin 10.8.0
  ~% g++-4.0 ~/test.cpp && ./a.out
  Size of bool: 1
  ~% g++-4.2 ~/test.cpp && ./a.out
  Size of bool: 1

> Somebody missed their own https://developer.mozilla.org/en/C++_Portability_Guide ;)

That document is completely out of date; unfortunately it's not being maintained.

Comment 4

[Stefan Baebler]

(In reply to Boris Zbarsky (:bz) from comment #3)
> > GCC even has "one-byte-bool" flag, because default gcc bool size on mac is 4 bytes.
> Except it's not:
>   Darwin 10.8.0
>   Size of bool: 1

Let me guess: this was ran on x86 CPU, right?

http://gcc.gnu.org/onlinedocs/gcc-4.2.0/gcc/Darwin-Options.html states:
-mone-byte-bool
Override the defaults for `bool' so that `sizeof(bool)==1'. By default `sizeof(bool)' is `4' when compiling for Darwin/PowerPC and `1' when compiling for Darwin/x86, so this option has no effect on x86.

Comment 5

[Boris Zbarsky [:bzbarsky]]

We don't support PowerPC anymore, so the PowerPC behavior is not a problem.  The TenFourFox project may want to add that compile-time option, though, depending...  Certainly the lack of ABI compat between PPC builds and x86 builds is not an issue.  ;)

Comment 6

[Matej Spiller-Muys]

> How would that help the issues you describe?  If it would, it would be worth doing.
If bool size was checked inside prtypes.h or some other firefox common header it would prevent people building their own xpcom components with a 
compiler that does not have the same bool size than firefox c++ compiler.
If you are using some old version of Visual C++ compiler, you would get false and true passed from firefox changed to true (99% of time) without any error or warning.
And those bugs are VERY hard to find.

For example even now I see bool defined as int inside nptypes.h:

#elif defined(_AIX) || defined(__sun) || defined(__osf__) || defined(IRIX) || defined(HPUX)
  /*
   * AIX and SunOS ship a inttypes.h header that defines [u]int32_t,
   * but not bool for C.
   */
  #include <inttypes.h>

  #ifndef __cplusplus
    typedef int bool;
    #define true   1
    #define false  0
  #endif 

> Except it's not:
True. It is default only on PowerPC.
from doc:
-mone-byte-bool
Override the defaults for `bool' so that `sizeof(bool)==1'. By default `sizeof(bool)' is `4' when compiling for Darwin/PowerPC and `1' when compiling for Darwin/x86, so this option has no effect on x86. 
Warning: The -mone-byte-bool switch causes GCC to generate code that is not binary compatible with code generated without that switch. 
Using this switch may require recompiling all other modules in a program, including system libraries. Use this switch to conform to a non-default data model. 


> not to mention creating an additional bar to new contributors and causing performance issues in some cases
you mean additional bar from clean: PRUint16, PRint16, PRBool, PR*.* to using PRUint16, PRint16, PR*.*,  and bool (sometimes) or PRBool (someother times when using NSS or NSPR). 
Or are you going to change all PR types to their native counterparts also? This would at least less likely break ABI & API (than bool change).
Performance issues? Can u point me to those performance issues? Are those even comparable to XPCOM overhead?

And I am even afraid to ask how are you going to break NSS or NSPR API in the future. There are both still using PRBool all over the place.

It would be far better if you would add another type to IDL and use new type internally and mark old boolean as deprecated (but still working as it is).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Or are you going to change all PR types to their native counterparts also?

We're generally moving in the direction of using the stdint types, yes.  That change should be ABI-compatible.

> Performance issues? Can u point me to those performance issues?

There were lots of profiles where conversion from bool to PRBool was showing up because we had to interface with existing code that used bool.  No direct pointers, sorry.  And the relevant code wasn't XPCOM, but was ending up with PRBools coming in via XPCOM.

> And I am even afraid to ask how are you going to break NSS or NSPR API in the future. 

No changes expected there.  They're C libraries, not C++, with an explicit goal of permanent API/ABI compat.  This last is decidedly a non-goal of XPCOM.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> > Or are you going to change all PR types to their native counterparts also?
> 
> We're generally moving in the direction of using the stdint types, yes. 
> That change should be ABI-compatible.

MSVC 10's <stdint.h> made a choice of underlying type for uint32_t that's different from how PRUint32 is represented (long != int or something).  A change there will affect linking behavior in pretty much the same way PRBool->bool affects it.  When compiling with versions of MSVC that don't have <stdint.h>, we could be compatible with prtypes.h if we wanted.  But that just puts off the day of reckoning; seems better to be forward-compatible with them, to me, as we've chosen to do.

The same sort of thing's happened on other platforms, too.  The BSDs ended up changing underlying type for PRUint64/uint64_t, see bug 634793.  On Linux I had to change all the call sites where PR{Ui,I}nt32* was being passed to JSAPI that expected {u,}int32_t*.

It's possible/likely the NSPR types and <stdint.h> types are compatible at the instruction level.  That'd be better than happened with the PRBool/bool change.  But there'd be as much C++ ABI incompatibility from different mangling behavior for int* versus long*, or long* versus long long*, or what-have-you.

But you gotta crack a few eggs to make an omelet.  If that's the price we pay for better code (but also fewer linker problems, as it turns out different parts of the codebase define sized integer types incompatibly, even for the exact same type name), the thinking has been: so be it, for the same reason we did the PRBool->bool change.

Comment 9

[Cameron Kaiser [:spectre]]

We're already building 10 with some slight modifications on OS X/ppc, so this wasn't a big problem for us (but I appreciate the heads up). We just had to correct some sizeof()s in a few places.

Comment 10

[Matej Spiller-Muys]

> MSVC 10's <stdint.h> made a choice of underlying type for uint32_t that's 
> different from how PRUint32 is represented (long != int or something).
ABI should be stable in MSVC 10 too. And long size equals to int size.
http://msdn.microsoft.com/en-us/library/s3f49ktz.aspx

> The same sort of thing's happened on other platforms, too.  
> The BSDs ended up changing underlying type for PRUint64/uint64_t, see bug 634793.
AFAIK API versus ABI compatibility. And I hope that u see my frustration.

Well I haven't even touched the API compatibility. How many problems will developers have when they want to support latest and older FF versions with the same code. Probably introducing yet another layer on top of PRBOOL/bool, confusing new contributors even more :) 
I have now solved this by basically replacing all occurrences of IDL boolean type to integer one. Making our code compilable and runnable by older and latest versions of FF (javascript can thankgod convert bool <-> int).

XPCOM is becoming too much of a moving target for us to support. Too many non backward compatible changes in only 1 year. We will probably need to switch to js-ctypes (no OOP :( ) or better NPAPI codegen like http://www.firebreath.org/display/documentation/FireBreath+Home or http://code.google.com/p/nixysa/, leaving XPCOM for your internal use/pleasure only.

Comment 11

[Cameron Kaiser [:spectre]]

There are a few places that *could* benefit from doing this a bit more portably. For example, in nsTextFrameThebes there are a number of memset()s that initialize arrays of bool assuming that sizeof(bool)==sizeof(char). Simply throwing in a n*sizeof(bool) would fix that on a case by case basis for all platforms. I don't think all the patches we've done are desirable upstream, but I think ones like this are worth it. Boris, if you agree, I'll file individual bugs and patches on the files that need such corrections.

Comment 12

[Cameron Kaiser [:spectre]]

(example: http://code.google.com/p/tenfourfox/issues/detail?id=129)

Comment 13

[Benjamin Smedberg]

It's not clear what action this bug is proposing, or whether it's just a complaint about the decision which was made previously. In any case I believe the decision was correct and there's no further action we would take on this bug. If there are specific code-level bugs which affect tenfourfox because of the 4-byte or 1-byte bool, please file them specifically. I agree that we shouldn't be assuming that bools are one-byte in cross-platform code.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Simply throwing in a n*sizeof(bool)

Should be done.  Please file bugs!

Comment 15

[Cameron Kaiser [:spectre]]

Will do!

Comment 16

[Matej Spiller-Muys]

The actions for this bug should be:
1.
since bool size depends on compiler (not standardized afaik) => there should be a check inside some common xpcom header that should check sizeof(bool) == 1. so that u know that bool is going to work when you build xpcom component.

2.
documentation update what is still PRBool and what is bool. how to convert between prbool and bool.

3.
documentation update, how to support bool and PRBool at the same time if u want to support older FFs. My solution is to just ditch bool and use integer type for bool.

4.
update c++ portability guide

Comment 17

[Benjamin Smedberg]

1) bool does depend on compiler options, but we don't want to require that it be a 1-byte type
2) We typically don't keep bugs on documentation fixes, the developer.mozilla.org site is a public documentation site that you can help us with.
3) By "support older FFs" do you mean a compile-time change in your code? We don't support having a single binary component that works in multiple versions of Firefox. If you want a compile-time change, you could just invent a typedef which is PRBool when compiling against an old version and bool when compiling against a new version...
4) fixed

Comment 18

[Matej Spiller-Muys]

1) actually it should depends on the compiler that you are using for each OS ... Windows 1 byte, some other os perhaps 4 bytes ... If I use old Visual Studio, I should get compile error that bool size is not the same as it is in the Firefox. Otherwise it is just chaos and bizarre runtime errors (and decreasing addons stability and firefox for that matter). This way you are also going to be safe, when some compiler vendor decides to change their bool size (like MS did in VC++).

2) I can help, but my solution was to simply drop bool usage and use int instead :) Not probably what you intended with all the PRbool/bool confusion.

3) I meant how to have single source code and compile with multiple FF SDKs. This was possible up until FF 10.
Perhaps:
#ifdef ???? >= 10  (how to check SDK's version?)
typedef PRBool bool
#endif

Of course this is not gonna work, if you use NSPR or NSS inside you XPCom extension. Then you are ******.

Comment 19

[Mark Smith [:mcs]]

(In reply to Matej Spiller-Muys from comment #18)
> 3) I meant how to have single source code and compile with multiple FF SDKs.
> This was possible up until FF 10.
> Perhaps:
> #ifdef ???? >= 10  (how to check SDK's version?)
> typedef PRBool bool
> #endif
> 
> Of course this is not gonna work, if you use NSPR or NSS inside you XPCom
> extension. Then you are ******.

You should introduce a new type.  Once you do that, things are pretty easy.  For the one remaining add-on that that my company maintains that uses a C++-based XPCOM component, we added something like this to a shared header file:

/* In Firefox 10, Mozilla switched from PRBool (an int) to C++'s bool. */
#if defined(XPCOM_USE_PRBOOL)
typedef PRBool XPCOMBool;
#else
typedef bool XPCOMBool;
#endif

Then, we replaced every occurrence of PRBool in our XPCOM component source code with XPCOMBool.

The final piece was for our Makefile to define XPCOM_USE_PRBOOL when building the component for older versions of Firefox (we do not have automatic detection for that; the Makefile just knows based on how we start the  build).

Comment 20

[Matej Spiller-Muys]

Mark. We use NSS and NSPR inside our XPCOM (for digital signature). NSS still needs original PRBool (and are going to need it in the future). By replacing PRBool with bool, it breaks NSS. If we don't typedef, it breaks XPCOM. That's why we replaced bool with int, leaving PRBool as it is and simply not use new bool.