Closed Bug 687338 Opened 8 years ago Closed 8 years ago

Optimized assignments to Vector<int,uint,Number> coerce value to element type out of order.

Categories

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: edwsmith, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Verifier case OP_setproperty, for known object types Vector.<int>, <uint>, and <Number>, explicitly coerce the value to the element type after the null check but before the range check.  For other element types, the coerce is after the range check (inside the setNativeXXXProperty() helper function).

Test case:

  function f(i:int, value) {
    var v:Vector.<int> = new Vector.<int>()
    v[i] = value
  }
  var value = { valueOf: function() { print("valueOf"); return 1 } }
  f(0, value)
  f(1, value)

With CodegenLIR:

  valueOf
  valueOf
  RangeError: Error #1125

With abc Interpreter

  valueOf
  RangeError: Error #1125

Which is right?  A quick scan of the vector speclet in specs/speclets/Vector.html did not specify the order of operations.  The code however implies the coerce should be before the range check:  (from specs/speclets/code/Vector.es):

  meta final function set(name, v): void {
    let idx = double(name);
    if (!intrinsic::isNaN(idx)) {
      let value: T = v;  // Note, effectful
      if (!isIntegral(idx) || idx < 0 || fixed && idx >= length ||
          !fixed && idx > length)
        throw new RangeError();
      informative::setValue(intrinsic::toUint(idx), value);
    } 
    else
      intrinsic::set(this, name, v);
  }

  informative function setValue(idx: double, val: T)
    storage[idx] = val;

If we take the es4 code as "spec", then in both forms of the set operation, the order is:

  nullcheck(vector)
  coerce(value -> T)
  rangecheck(index)

Conclusions

  - The helper functions used by the interpreter get it wrong, and also the JITs get it wrong when the vector type is not known since they use generic helpers which get it wrong.  The fix to the helpers is to coerce to the element type before the range check.
  - The Verifier's logic for <int,uint,Number> get it right by coercing explicitly, but wrong for other element types.  CodegenLIR's correctness derives from the Verifier logic.
  - Bug 678952 improves the verifier's logic for Vector.<C> to match <int> (etc), so the Vector<C> case's wrongness will change when bug 678952 lands.

This bug doesn't address the order of converting the index, but inferring from the same code, the overall order of effects should be:

  nullcheck(vector)
  coerce(index -> Number)
  if isNaN(index) throw property error ? (need clarification)
  coerce(value -> T)
  rangecheck(index)
I probably wrote the ES4 code.  I do not have a clear memory of considering the order of effects in that code when I wrote it but the comment in the code suggests that I did.

I'm trying to think if we have a precedent elsewhere in the language but nothing really springs to mind.  It would have to be something like a fixture defined by a getter without a setter.
Attached file Comprehensive test case (obsolete) —
Straightforward fix, though I was concerned about missing other paths.  Test case deserves some scrutiny.
Attachment #601188 - Flags: review?(edwsmith)
Note that the verifier changes in the fix for bug 678952 were versioned.  There, it was possible that the change would affect the way names resolved in some cases.  In the present fix, we've only altered the ordering of side effects in toString() and toValue() methods, which typically would have only benign side effects (if any at all) but are not formally constrained in this manner.

Versioning these virtual methods without a performance hit on each vector property set would require subclassing each of the TypedVectorObject classes with an old and a new variant, as well as more straightforward code generator changes.
Attachment #601188 - Flags: review?(edwsmith) → review+
I had to study the test code a bit to understand the state-change logic, a short comment would help.  otherwise, nothing looks obviously wrong.
Attachment #601186 - Attachment is obsolete: true
Attachment #601424 - Attachment description: Comprehensive test case, with float support (with added comments) → Comprehensive test case (with added comments)
changeset: 7231:42e0f73a6bf9
user:      William Maddox <wmaddox@adobe.com>
summary:   Bug 687338: Consistently coerce value prior to range check when setting Vector property  (r=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/42e0f73a6bf9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.