Closed Bug 975373 Opened 6 years ago Closed 6 years ago

Old out-parameter type used by some inlining instructions in jit/CodeGenerator.cpp

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: romain.perier, Assigned: romain.perier)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140220030202

Steps to reproduce:

Hello,

RegExpExec and BinaryV use a Value * as out-param type, this is the old way.
All VMFunction propotypes should use MutableHandleValue instead: 
- It might be hard to understand (when should we root the returned value?)
- There wouldn't be a unique way to do so (not easy to understand, prone to bugs, etc..)
- as discussed on IRC: "Ion functions have a zero-cost Rooting mechanism as opposed to Rooted, so we should just refuse to have any Value* now."

I will propose a patch to fix these problems
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → romain.perier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch version 1 (obsolete) — Splinter Review
See the proposed patch in attachment
Attachment #8380367 - Flags: review?(hv1989)
Comment on attachment 8380367 [details] [diff] [review]
patch version 1

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

I have two simplifications:
1) use out.setType(); instead of out.address()->setType() everywhere. I have highlighted some, but you can do that everywhere.
2) use REGS.stackHandleAt(-2) instead of MutableHandleValue::fromMarkedLocation(&REGS.sp[-2]); everywhere. Again I only highlighted some, but need to get done everywhere

Thanks!

::: js/src/jit/ParallelFunctions.cpp
@@ +549,5 @@
>          return false;
>      if (!NonObjectToUint32(cx, lhs, &left) || !NonObjectToInt32(cx, rhs, &right))
>          return false;
>      left >>= right & 31;
> +    out.address()->setNumber(uint32_t(left));

out.setNumber(...);

::: js/src/vm/Interpreter-inl.h
@@ +628,5 @@
>      int32_t  right;
>      if (!ToUint32(cx, lhs, &left) || !ToInt32(cx, rhs, &right))
>          return false;
>      left >>= right & 31;
> +    out.address()->setNumber(uint32_t(left));

out.setNumber(...)

::: js/src/vm/Interpreter.cpp
@@ +1209,5 @@
>      if (lhs.isInt32() && rhs.isInt32()) {
>          int32_t l = lhs.toInt32(), r = rhs.toInt32();
>          int32_t t;
>          if (MOZ_LIKELY(SafeAdd(l, r, &t))) {
> +            res.address()->setInt32(t);

res.setInt32(...)

@@ +2198,5 @@
>  CASE(JSOP_URSH)
>  {
>      HandleValue lval = REGS.stackHandleAt(-2);
>      HandleValue rval = REGS.stackHandleAt(-1);
> +    MutableHandleValue res = MutableHandleValue::fromMarkedLocation(&REGS.sp[-2]);

REGS.stackHandleAt(-2);

@@ +2209,5 @@
>  CASE(JSOP_ADD)
>  {
>      MutableHandleValue lval = REGS.stackHandleAt(-2);
>      MutableHandleValue rval = REGS.stackHandleAt(-1);
> +    MutableHandleValue res = MutableHandleValue::fromMarkedLocation(&REGS.sp[-2]);

REGS.stackHandleAt(-2)
Attachment #8380367 - Flags: review?(hv1989)
Attachment #8380367 - Flags: feedback+
Attached patch patch version 2 (obsolete) — Splinter Review
Attachment #8380367 - Attachment is obsolete: true
Attachment #8380654 - Flags: review?(hv1989)
Everything should be fixed
Attached patch Patch version 3 (rebased) (obsolete) — Splinter Review
Attachment #8380654 - Attachment is obsolete: true
Attachment #8380654 - Flags: review?(hv1989)
Attached patch Patch version 3Splinter Review
Attachment #8380668 - Flags: review?(hv1989)
Attachment #8380666 - Attachment is obsolete: true
Attachment #8380668 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/95325311fa2e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.