PRBool was removed from Gecko about 18 months ago (bug 675553). Time for JSBool to go as well. I think the only downside is the API changes. There will be many, but they will be trivial. And then we can stop wasting time with gradual JSBool conversions (e.g. bug 695908, bug 759902, nuh 765913, bug 817724).

Luke, before I go ahead with it -- are you happy with this?

Is bool support in the JITs not anymore a problem?

Oh, I didn't realize there were issues with the JITs...

(In reply to Nicholas Nethercote [:njn] from comment #3) > Oh, I didn't realize there were issues with the JITs... The problem is that if a function returns bool instead of JSBool, C++ compilers are free to set only the lower byte(s) and leave the other bits garbage. This means the JITs should only test the low bits. See branchIfFalseBool in IonMacroAssembler.h. To work around this, there are a number of places where we call stubs that return a JSBool, see for instance IsDelegateObject. We could change these to use uint32_t (or a typedef) or use bool + branchIfFalseBool or something.

I wonder if it's worth changing JSBool to, say, js::jitbool, and using it in the places where the JITs need it, and using bool everywhere else. It'll be harder to do than the global search-and-replace I was hoping for, but at least every JSBool occurrence would get audited.

Yeah that would be very nice. You can search for JSBool in ion/* and change these to js::jitbool. Most of them will look like this: typedef bool (*HasInstanceFn)(JSContext *, HandleObject, HandleValue, JSBool *); static const VMFunction HasInstanceInfo = FunctionInfo<HasInstanceFn>(js::HasInstance); So you know you will have to change js::HasInstance. Once that's done, a global s/JSBool/bool should be fine.

Oh you also have to look at all masm.callWithABI calls: masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, ObjectEmulatesUndefined)); ObjectEmulatesUndefined returns JSBool, that should be changed to js::jitbool. That + comment 6 should cover everything, as far as I know.

(In reply to Nicholas Nethercote [:njn] from comment #1) Yeah; js::jitbool sounds like a good way to deal with the jit problem.

Oh, my one request though is that we avoid making a lot of VM-wide functions use js::jitbool; ideally I think these would call a wrapper function in ion/ that then called VM-wide functions.

Do we want to change the calling convention for the WebIDL specialized methods/getters/setters to return jitbool too?

I'm going to say what I *hope* everyone is thinking and note that js::jitbool seems useful only as a stepping stone permitting quicker elimination of JSBool. Once that's done, we should implement JIT support for bool, then we should convert js::jitbool to bool. And in the end, everyone would be using the fast, efficient, nice bool stuff. Where that would leave WebIDL stuff once JSBool is gone, I'd expect, would depend on conditions on the ground after JSBool's removal. Ditto for the timeline/timeframe for any of the intermediate steps toward removing js::jitbool.

Whenever we use callVM / VMFunction, we end up calling a small trampoline ("VMWrapper") for that VM function. We could have that trampoline "extend" bool outparams to 32-bit. That only leaves places where we use masm.callWithABI. These could use branchIfFalseBool or something similar. That would avoid js::jitbool and extra C++ wrappers completely. I can do this part if you want.

> I can do this part if you want. That'd be good, thanks. I started by changing HasInstanceFn and then propagated the changes outwards to fix the compile errors, and it affected JS_HasInstance() and Class::hasInstance, which didn't seem ideal. A change that contains the JIT-specific stuff within a small part of ion/ would be better.

Fwiw, right now the webidl bits use a bool return value. And we clearly have JIT support for that. We didn't use to handle the return value correctly there in the JIT, but had to fix it because it caused bugs... The one drawback is that a bool return value is slower to test for being false than a JSBool one: you have to mask and test, not just test.

(In reply to Boris Zbarsky (:bz) from comment #14) > The one drawback is that a bool return value is slower to test for being > false than a JSBool one: you have to mask and test, not just test. There's a testb isntruction though.. We should probably use that on x86/x64 instead of the testl 0xff we have now.

Bug 899017 is on inbound now. If you s/JSBool/bool and apply the second patch in that bug, there shouldn't be any JIT problems.

Thanks! I'll get to it soon.

So I tried the simple global search-and-replace, combined with Jan's patch, and I get tons of jit-test failures. I haven't even tried building the browser yet. I've attached the combined patch. A more laborious, piece-by-piece approach may be required. I'm not that surprised; these kinds of changes are always harder than they first appear...

(In reply to Nicholas Nethercote [:njn] from comment #18) > So I tried the simple global search-and-replace, combined with Jan's patch, > and I get tons of jit-test failures. I haven't even tried building the > browser yet. Does it also fail if you disable the JITs? (--no-baseline)

> Does it also fail if you disable the JITs? (--no-baseline) I tried a couple. They still failed :( Time to start doing this in pieces.

(In reply to Nicholas Nethercote [:njn] from comment #21) > https://tbpl.mozilla.org/?tree=Try&rev=a13f00823a12 Whoops... that comment was meant for bug 902332.

I worked out the problem mentioned in comment 18 -- some slightly non-trivial changes are needed to js/public/Value.h. With that done, the global s/JSBool/bool/ works! Patches coming shortly.

(Same dance as in bug 902332: scripted changes first, then manual changes, then the two combined.)

The important thing this patch does is fix up |Value|. - Changes the |boo| field of 32-bit |Value|s to int32_t. For some reason I can't work out, if that field is a bool then things break. I'm guessing that for some reason the payload types must be four bytes. (I guess the assertion that |sizeof(bool) == 4|, which I've removed, is related somehow.) - Removes the now-unnecessary assertions in BOOLEAN_TO_JSVAL_IMPL. - Uses the payload mask in 64-bit JSVAL_TO_BOOLEAN_IMPL. This wasn't needed prevoiusly, because if you cast a uint64_t to JSBool (i.e. int) it just uses the bottom 32-bits, i.e. it ignores the tag. But for bool, the entire value is looked at, so we have to mask out the tag. - Removes the declaration of JSBool. The 32-bit big-endian changes won't get any testing on TBPL, unfortunately. But AFAICT the difference between the 32-bit big- and little-endian |Value|s is just the order of the main fields, so fingers crossed... The patch also does some assorted housekeeping. - Removes the |returnType| parameter to CGAbstractBindingMethod.__init__, because they're now always |bool|. - Fixes up some comments (including removing a "njn" comment that I left behind in a previous patch). - Fixes tons of indentation. I found these by searching for certain patterns, rather than checking every line of the patch, so I might have missed some.

Waldo, I guess this is worth mentioning in https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 ?

Cool! Not very urgent, but for bug 899017 I had to add some temporary JSBool locals to pass to functions that still required a JSBool *. We can remove these now and pass the bool * argument directly. See JS_HasInstance, js::HasInstance, js::DeleteProperty and js::DeleteElement (In reply to Nicholas Nethercote [:njn] from comment #26) > - Changes the |boo| field of 32-bit |Value|s to int32_t. For some reason I > can't work out, if that field is a bool then things break. I'm guessing > that > for some reason the payload types must be four bytes. The JITs want this as well: ToInt32(BooleanValue) will just load the 4-byte payload and assume it's 0 or 1.

Comment on attachment 787405 [details] [diff] [review] (combined) Nice work slogging through all this.

Comment on attachment 787587 [details] [diff] [review] (part 2) - Avoid some bool shuffling. Review of attachment 787587 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 787405 [details] [diff] [review] (combined) r=me on the manual parts; I mostly assumed the automatic parts work and did some spot-checking for us doing things like having non-boolean JSBools in the old world (thankfully, we don't seem to).

Yep, definitely worth mentioning there. Technically every doc needs to be updated to bool from JSBool, but the <stdint.h> precedent (we used uint32 before we used <stdint.h> uint32_t, and so on) says we can just not do that, and have the 31-note as good enough. :-) At some point once 31 is new enough we can go and actually change everything, or something.

Here's a green try run, BTW: https://tbpl.mozilla.org/?tree=Try&rev=c3579e2bc2df The |bc| failures are due to a bad patch that landed just before my push.

Fingers crossed this sticks: https://hg.mozilla.org/integration/mozilla-inbound/rev/7db702296585 https://hg.mozilla.org/integration/mozilla-inbound/rev/18236f0722de

https://hg.mozilla.org/mozilla-central/rev/7db702296585 https://hg.mozilla.org/mozilla-central/rev/18236f0722de

Here's the SM31 docs diff: https://developer.mozilla.org/en-US/docs/SpiderMonkey/31$compare?to=453209&from=449741