899017 - Fix VM functions called by the JITs to use bool instead of JSBool

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

See bug 898914.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This changes all VM functions that are called by the JITs to use bool instead of JSBool. For callVM, the VM wrapper zero-extends bool to int32.

I also audited all callWithABI calls and changed these to use branchIfFalseBool or similar functions that work on bool instead of JSBool.

With this patch, s/JSBool/bool (bug 898914) should not break the JITs.

Looks good on Try:

https://tbpl.mozilla.org/?tree=Try&rev=175b830f659c
Fix for ARM compile error: https://tbpl.mozilla.org/?tree=Try&rev=132cf573b36a

(Full Try push since this depends on the C++ compiler / platform etc.)

Comment 2

[Jan de Mooij [:jandem]]

This patch uses branchIfFalseBool in a few places for getter/setter/native calls that currently still return JSBool.

bz pointed out that this will break if we return a JSBool value other than 0 or 1 (this can happen if we "return foo & bar" somewhere).

I will remove these from the patch and post a small patch njn can add to his mass-rename patch.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch v1

Reverted the changes to IonCaches.cpp and the CallNative code in CodeGenerator::visitCallNative and Baseline's Call_Native stub.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch to land with bug 898914

njn, you should fold this patch into the mass-rename patch. See comment 2 for more info.

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 782686 [details] [diff] [review]
Patch v1

Review of attachment 782686 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/VMFunctions.cpp
@@ +201,5 @@
>      bool equal;
>      if (!js::EqualStrings(cx, lhs, rhs, &equal))
>          return false;
>      *res = (equal == Equal);
>      return true;

It looks like this should just be "return js::EqualStrings(cx, lhs, rhs, res);" -- but I'm not sure what's going on here with comparing a boolean to a Condition. It looks suspect.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91837985ae91

Comment 7

[Jan de Mooij [:jandem]]

Thanks for the quick reviews btw!

(In reply to Sean Stangl [:sstangl] from comment #5)
> It looks like this should just be "return js::EqualStrings(cx, lhs, rhs,
> res);" -- but I'm not sure what's going on here with comparing a boolean to
> a Condition. It looks suspect.

Hm yeah, I simplified it a bit. If (!Equal) we need to negate the result but we don't need a second bool variable.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91837985ae91