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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Romain Perier]

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

Comment 1

[Romain Perier]

Attachment: patch version 1

See the proposed patch in attachment

Comment 2

[Hannes Verschore [:h4writer]]

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)

Comment 3

[Romain Perier]

Attachment: patch version 2

Comment 4

[Romain Perier]

Everything should be fixed

Comment 5

[Romain Perier]

Attachment: Patch version 3 (rebased)

Comment 6

[Romain Perier]

Attachment: Patch version 3

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95325311fa2e

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95325311fa2e