IonMonkey: Remove bailing in LCallGeneric

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 years ago
Currently we are bailing in LCallGeneric when object is not a JSFunction. But actually that can use the same vm path as not compiled functions.
If we do so, then we need to be careful and introduce a primitive which is compiled into a bailout MIR, such as we can keep the test suite working.
Assignee

Comment 2

6 years ago
Posted patch Patch for ff24 (obsolete) — Splinter Review
This patch fixes the issue. But I haven't tested it well enough to mark for review. Atm I'm using it as WIP patch. There are no issues I'm aware of, though
Assignee

Updated

6 years ago
Blocks: 897962
Assignee

Comment 3

6 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> If we do so, then we need to be careful and introduce a primitive which is
> compiled into a bailout MIR, such as we can keep the test suite working.

Can you elaborate more about this? Do currently depend on this bailing mechanism in LGenericCall ?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Hannes Verschore [:h4writer] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #1)
> > If we do so, then we need to be careful and introduce a primitive which is
> > compiled into a bailout MIR, such as we can keep the test suite working.
> 
> Can you elaborate more about this? Do currently depend on this bailing
> mechanism in LGenericCall ?

The test suite use the fact that we do not yet handle calls to proxy to create a reproducible bailout:

  var bailout = Proxy.createFunction({}, Math.sin);

  function f2(a) {
      bailout();
      return f2.arguments;
  };
Flags: needinfo?(nicolas.b.pierron)
Assignee

Updated

6 years ago
Depends on: 899735
Assignee

Comment 5

6 years ago
I got one jit-test failure, due to error reporting difference. Should this get solved, or is this ok?

jit-test/tests/jaeger/bug588338.js

Without patch:
TypeError: [1, 2, 3] is not a function

With patch:
TypeError: [1, 2, 3](...) is not a function
Assignee: general → hv1989
Attachment #775116 - Attachment is obsolete: true
Attachment #783639 - Flags: review?(sstangl)
Comment on attachment 783639 [details] [diff] [review]
bug891910-bailing-lcallgeneric

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

TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy?

::: js/src/ion/CodeGenerator.cpp
@@ +1636,5 @@
>      Register objreg    = ToRegister(call->getTempObject());
>      Register nargsreg  = ToRegister(call->getNargsReg());
>      uint32_t unusedStack = StackOffsetOfPassedArg(call->argslot());
>      ExecutionMode executionMode = gen->info().executionMode();
> +    Label uncompiled, uncompiled2, thunk, makeCall, end;

"uncompiled2" is unused.

@@ +1654,3 @@
>      masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_));
> +    //if (!bailoutIf(Assembler::NotEqual, call->snapshot()))
> +    //    return false;

There's a bunch of leftover cruft here.

@@ +1654,4 @@
>      masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_));
> +    //if (!bailoutIf(Assembler::NotEqual, call->snapshot()))
> +    //    return false;
> +    masm.j(Assembler::NotEqual, &uncompiled);

cmpPtr(); j(); is otherwise known as branchPtr(). There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM.

::: js/src/ion/VMFunctions.cpp
@@ +80,5 @@
> +    } else {
> +        RootedValue rv(cx);
> +        if (!Invoke(cx, thisv, ObjectValue(*obj), argc, argvWithoutThis, &rv))
> +            return false;
> +        *rval = rv;

Although GetPCScript() and Monitor() cannot GC, it still seems strange to drop the Root on rv/rval early.

::: js/src/ion/VMFunctions.h
@@ +567,5 @@
>              cx_->runtime()->setIonReturnOverride(*rval_);
>      }
>  };
>  
> +bool InvokeFunction(JSContext *cx, HandleObject obj, uint32_t argc, Value *argv, Value *rval);

The argument is called "obj0" in VMFunctions.cpp.
Attachment #783639 - Flags: review?(sstangl)
Assignee

Comment 7

6 years ago
This addresses comments.

> TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy?
Apparently we don't need to worry about it. This still gives the same error when compiling --enable-more-deterministic (what fuzzers use). So I only had to patch that one script so it still checks the error, but not that tight.

> There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM.
This doesn't force a bailout through CallVM. This just calls the function using callVM, so we don't bail! I renamed the "uncompiled" label to "invoke". Explains it better.
Attachment #783639 - Attachment is obsolete: true
Attachment #786478 - Flags: review?(sstangl)
Assignee

Comment 8

6 years ago
Review ping
Flags: needinfo?(sstangl)
Comment on attachment 786478 [details] [diff] [review]
bug891910-bailing-lcallgeneric

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

::: js/src/ion/VMFunctions.cpp
@@ +19,5 @@
>  #include "ion/BaselineFrame-inl.h"
>  #include "vm/Interpreter-inl.h"
>  #include "vm/StringObject-inl.h"
>  
> +//TODO: make style???

:-P
Attachment #786478 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/9179e5001bb4
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 905396
Assignee

Updated

6 years ago
Depends on: 958228
You need to log in before you can comment on or make changes to this bug.