Closed Bug 899017 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

See bug 898914.
Attached patch Patch (obsolete) — Splinter Review
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.)
Attachment #782604 - Flags: review?(sstangl)
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.
Attached patch Patch v1Splinter Review
Reverted the changes to IonCaches.cpp and the CallNative code in CodeGenerator::visitCallNative and Baseline's Call_Native stub.
Attachment #782604 - Attachment is obsolete: true
Attachment #782604 - Flags: review?(sstangl)
Attachment #782686 - Flags: review?(sstangl)
njn, you should fold this patch into the mass-rename patch. See comment 2 for more info.
Attachment #782688 - Flags: review?(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.
Attachment #782686 - Flags: review?(sstangl) → review+
Attachment #782688 - Flags: review?(sstangl) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/91837985ae91
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.