Bug 1574415 - Part 1: Move argument conversion for StoreTypedElement to happen before range checks. r=jandem
authorAndré Bargull <andre.bargull@gmail.com>
Mon, 07 Oct 2019 11:56:27 +0000
changeset 496553 d2008fdba4af17f45aad4d2ed3f99b21296fb248
parent 496552 a0d28ade0bb377f5e99b1ac4b3b87edb535c96c2
child 496554 6067d1828df8f7ca054c4a57430e466e1987aa75
push id97326
push userarchaeopteryx@coole-files.de
push dateMon, 07 Oct 2019 16:46:32 +0000
treeherderautoland@144ebbca6844 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1574415
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1574415 - Part 1: Move argument conversion for StoreTypedElement to happen before range checks. r=jandem This ensures CacheIR doesn't completely ignore out-of-bounds writes to TypedArray objects and also enables to use additional scratch registers for bounds-checking. Differential Revision: https://phabricator.services.mozilla.com/D42810
js/src/jit-test/tests/auto-regress/bug1574415.js
js/src/jit/BaselineCacheIRCompiler.cpp
js/src/jit/CacheIR.cpp
js/src/jit/CacheIR.h
js/src/jit/IonCacheIRCompiler.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/auto-regress/bug1574415.js
@@ -0,0 +1,11 @@
+var err = 0;
+
+for (let x of [0, Object.create(null)]) {
+    try {
+        (new Int8Array)[1] = x;
+    } catch (e) {
+        err += 1;
+    }
+}
+
+assertEq(err, 1);
--- a/js/src/jit/BaselineCacheIRCompiler.cpp
+++ b/js/src/jit/BaselineCacheIRCompiler.cpp
@@ -414,16 +414,71 @@ bool BaselineCacheIRCompiler::emitGuardS
     return false;
   }
 
   Address addr(stubAddress(reader.stubOffset()));
   masm.branchPtr(Assembler::NotEqual, addr, sym, failure->label());
   return true;
 }
 
+bool BaselineCacheIRCompiler::emitGuardToInt32ModUint32() {
+  JitSpew(JitSpew_Codegen, __FUNCTION__);
+  ValueOperand value = allocator.useValueRegister(masm, reader.valOperandId());
+  Register output = allocator.defineRegister(masm, reader.int32OperandId());
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  Label notInt32;
+  masm.branchTestInt32(Assembler::NotEqual, value, &notInt32);
+  masm.unboxInt32(value, output);
+
+  Label done;
+  masm.jump(&done);
+
+  // If the value is a double, truncate; else, jump to failure.
+  masm.bind(&notInt32);
+  masm.branchTestDouble(Assembler::NotEqual, value, failure->label());
+  masm.unboxDouble(value, FloatReg0);
+  masm.branchTruncateDoubleMaybeModUint32(FloatReg0, output, failure->label());
+
+  masm.bind(&done);
+  return true;
+}
+
+bool BaselineCacheIRCompiler::emitGuardToUint8Clamped() {
+  JitSpew(JitSpew_Codegen, __FUNCTION__);
+  ValueOperand value = allocator.useValueRegister(masm, reader.valOperandId());
+  Register output = allocator.defineRegister(masm, reader.int32OperandId());
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  Label notInt32;
+  masm.branchTestInt32(Assembler::NotEqual, value, &notInt32);
+  masm.unboxInt32(value, output);
+  masm.clampIntToUint8(output);
+
+  Label done;
+  masm.jump(&done);
+
+  // If the value is a double, clamp to uint8; else, jump to failure.
+  masm.bind(&notInt32);
+  masm.branchTestDouble(Assembler::NotEqual, value, failure->label());
+  masm.unboxDouble(value, FloatReg0);
+  masm.clampDoubleToUint8(FloatReg0, output);
+
+  masm.bind(&done);
+  return true;
+}
+
 bool BaselineCacheIRCompiler::emitLoadValueResult() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   AutoOutputRegister output(*this);
   masm.loadValue(stubAddress(reader.stubOffset()), output.valueReg());
   return true;
 }
 
 bool BaselineCacheIRCompiler::emitLoadFixedSlotResult() {
@@ -1443,59 +1498,80 @@ bool BaselineCacheIRCompiler::emitArrayP
   masm.tagValue(JSVAL_TYPE_INT32, scratchLength, val);
 
   return true;
 }
 
 bool BaselineCacheIRCompiler::emitStoreTypedElement() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   Register obj = allocator.useRegister(masm, reader.objOperandId());
-  Register index = allocator.useRegister(masm, reader.int32OperandId());
-  ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
-
   TypedThingLayout layout = reader.typedThingLayout();
   Scalar::Type type = reader.scalarType();
+  Register index = allocator.useRegister(masm, reader.int32OperandId());
+
+  Maybe<Register> valInt32;
+  Maybe<NumberOperandId> valFloat;
+  switch (type) {
+    case Scalar::Int8:
+    case Scalar::Uint8:
+    case Scalar::Int16:
+    case Scalar::Uint16:
+    case Scalar::Int32:
+    case Scalar::Uint32:
+    case Scalar::Uint8Clamped:
+      valInt32.emplace(allocator.useRegister(masm, reader.int32OperandId()));
+      break;
+
+    case Scalar::Float32:
+    case Scalar::Float64:
+      valFloat.emplace(reader.numberOperandId());
+      break;
+
+    case Scalar::BigInt64:
+    case Scalar::BigUint64:
+    case Scalar::MaxTypedArrayViewType:
+    case Scalar::Int64:
+      MOZ_CRASH("Unsupported TypedArray type");
+  }
+
   bool handleOOB = reader.readBool();
 
   AutoScratchRegister scratch1(allocator, masm);
+  AutoScratchRegister scratch2(allocator, masm);
 
   FailurePath* failure;
   if (!addFailurePath(&failure)) {
     return false;
   }
 
   // Bounds check.
   Label done;
   LoadTypedThingLength(masm, layout, obj, scratch1);
 
-  // Unfortunately we don't have more registers available on x86, so use
-  // InvalidReg and emit slightly slower code on x86.
-  Register spectreTemp = InvalidReg;
-  masm.spectreBoundsCheck32(index, scratch1, spectreTemp,
+  masm.spectreBoundsCheck32(index, scratch1, scratch2,
                             handleOOB ? &done : failure->label());
 
   // Load the elements vector.
   LoadTypedThingData(masm, layout, obj, scratch1);
 
   BaseIndex dest(scratch1, index, ScaleFromElemWidth(Scalar::byteSize(type)));
 
-  // Use ICStubReg as second scratch register. TODO: consider doing the RHS
-  // type check/conversion as a separate IR instruction so we can simplify
-  // this.
-  Register scratch2 = ICStubReg;
-  masm.push(scratch2);
-
-  Label fail;
-  StoreToTypedArray(cx_, masm, type, val, dest, scratch2, &fail);
-  masm.pop(scratch2);
-  masm.jump(&done);
-
-  masm.bind(&fail);
-  masm.pop(scratch2);
-  masm.jump(failure->label());
+  if (type == Scalar::Float32 || type == Scalar::Float64) {
+    allocator.ensureDoubleRegister(masm, *valFloat, FloatReg0);
+
+    if (type == Scalar::Float32) {
+      ScratchFloat32Scope fpscratch(masm);
+      masm.convertDoubleToFloat32(FloatReg0, fpscratch);
+      masm.storeToTypedFloatArray(type, fpscratch, dest);
+    } else {
+      masm.storeToTypedFloatArray(type, FloatReg0, dest);
+    }
+  } else {
+    masm.storeToTypedIntArray(type, *valInt32, dest);
+  }
 
   masm.bind(&done);
   return true;
 }
 
 bool BaselineCacheIRCompiler::emitCallNativeSetter() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   Register obj = allocator.useRegister(masm, reader.objOperandId());
--- a/js/src/jit/CacheIR.cpp
+++ b/js/src/jit/CacheIR.cpp
@@ -3884,22 +3884,16 @@ AttachDecision SetPropIRGenerator::tryAt
   if (!obj->is<TypedArrayObject>() && !IsPrimitiveArrayTypedObject(obj)) {
     return AttachDecision::NoAction;
   }
 
   if (!rhsVal_.isNumber()) {
     return AttachDecision::NoAction;
   }
 
-  // bigIntArray[index] = rhsVal_ will throw as the RHS is a number.
-  if (obj->is<TypedArrayObject>() &&
-      Scalar::isBigIntType(obj->as<TypedArrayObject>().type())) {
-    return AttachDecision::NoAction;
-  }
-
   bool handleOutOfBounds = false;
   if (obj->is<TypedArrayObject>()) {
     handleOutOfBounds = (index >= obj->as<TypedArrayObject>().length());
   } else {
     // Typed objects throw on out of bounds accesses. Don't attach
     // a stub in this case.
     if (index >= obj->as<TypedObject>().length()) {
       return AttachDecision::NoAction;
@@ -3910,24 +3904,56 @@ AttachDecision SetPropIRGenerator::tryAt
     if (cx_->zone()->detachedTypedObjects) {
       return AttachDecision::NoAction;
     }
   }
 
   Scalar::Type elementType = TypedThingElementType(obj);
   TypedThingLayout layout = GetTypedThingLayout(obj->getClass());
 
+  // bigIntArray[index] = rhsVal_ will throw as the RHS is a number.
+  if (Scalar::isBigIntType(elementType)) {
+    return AttachDecision::NoAction;
+  }
+
   if (IsPrimitiveArrayTypedObject(obj)) {
     writer.guardNoDetachedTypedObjects();
     writer.guardGroupForLayout(objId, obj->group());
   } else {
     writer.guardShapeForClass(objId, obj->as<TypedArrayObject>().shape());
   }
 
-  writer.storeTypedElement(objId, indexId, rhsId, layout, elementType,
+  Maybe<OperandId> rhsValId;
+  switch (elementType) {
+    case Scalar::Int8:
+    case Scalar::Uint8:
+    case Scalar::Int16:
+    case Scalar::Uint16:
+    case Scalar::Int32:
+    case Scalar::Uint32:
+      rhsValId.emplace(writer.guardToInt32ModUint32(rhsId));
+      break;
+
+    case Scalar::Float32:
+    case Scalar::Float64:
+      rhsValId.emplace(writer.guardIsNumber(rhsId));
+      break;
+
+    case Scalar::Uint8Clamped:
+      rhsValId.emplace(writer.guardToUint8Clamped(rhsId));
+      break;
+
+    case Scalar::BigInt64:
+    case Scalar::BigUint64:
+    case Scalar::MaxTypedArrayViewType:
+    case Scalar::Int64:
+      MOZ_CRASH("Unsupported TypedArray type");
+  }
+
+  writer.storeTypedElement(objId, layout, elementType, indexId, *rhsValId,
                            handleOutOfBounds);
   writer.returnFromIC();
 
   if (handleOutOfBounds) {
     attachedTypedArrayOOBStub_ = true;
   }
 
   trackAttached(handleOutOfBounds ? "SetTypedElementOOB" : "SetTypedElement");
--- a/js/src/jit/CacheIR.h
+++ b/js/src/jit/CacheIR.h
@@ -213,16 +213,18 @@ extern const uint32_t ArgLengths[];
   _(GuardIsUndefined, Id)                                                      \
   _(GuardToBoolean, Id, Id)                                                    \
   _(GuardToString, Id)                                                         \
   _(GuardToSymbol, Id)                                                         \
   _(GuardToBigInt, Id)                                                         \
   _(GuardIsNumber, Id)                                                         \
   _(GuardToInt32, Id, Id)                                                      \
   _(GuardToInt32Index, Id, Id)                                                 \
+  _(GuardToInt32ModUint32, Id, Id)                                             \
+  _(GuardToUint8Clamped, Id, Id)                                               \
   _(GuardType, Id, Byte)                                                       \
   _(GuardShape, Id, Field)                                                     \
   _(GuardGroup, Id, Field)                                                     \
   _(GuardProto, Id, Field)                                                     \
   _(GuardClass, Id, Byte)     /* Guard per GuardClassKind */                   \
   _(GuardAnyClass, Id, Field) /* Guard an arbitrary class */                   \
   _(GuardCompartment, Id, Field, Field)                                        \
   _(GuardIsExtensible, Id)                                                     \
@@ -287,17 +289,17 @@ extern const uint32_t ArgLengths[];
   _(AddAndStoreDynamicSlot, Id, Field, Id, Byte, Field, Field)                 \
   _(AllocateAndStoreDynamicSlot, Id, Field, Id, Byte, Field, Field, Field)     \
   _(StoreTypedObjectReferenceProperty, Id, Field, Byte, Byte, Id)              \
   _(StoreTypedObjectScalarProperty, Id, Field, Byte, Byte, Id)                 \
   _(StoreDenseElement, Id, Id, Id)                                             \
   _(StoreDenseElementHole, Id, Id, Id, Byte)                                   \
   _(ArrayPush, Id, Id)                                                         \
   _(ArrayJoinResult, Id)                                                       \
-  _(StoreTypedElement, Id, Id, Id, Byte, Byte, Byte)                           \
+  _(StoreTypedElement, Id, Byte, Byte, Id, Id, Byte)                           \
   _(CallNativeSetter, Id, Id, Field)                                           \
   _(CallScriptedSetter, Id, Field, Id, Byte)                                   \
   _(CallSetArrayLength, Id, Byte, Id)                                          \
   _(CallProxySet, Id, Id, Field, Byte)                                         \
   _(CallProxySetByValue, Id, Id, Id, Byte)                                     \
   _(CallAddOrUpdateSparseElementHelper, Id, Id, Id, Byte)                      \
   _(CallInt32ToString, Id, Id)                                                 \
   _(CallNumberToString, Id, Id)                                                \
@@ -845,16 +847,30 @@ class MOZ_RAII CacheIRWriter : public JS
 
   Int32OperandId guardToInt32Index(ValOperandId val) {
     Int32OperandId res(nextOperandId_++);
     writeOpWithOperandId(CacheOp::GuardToInt32Index, val);
     writeOperandId(res);
     return res;
   }
 
+  Int32OperandId guardToInt32ModUint32(ValOperandId val) {
+    Int32OperandId res(nextOperandId_++);
+    writeOpWithOperandId(CacheOp::GuardToInt32ModUint32, val);
+    writeOperandId(res);
+    return res;
+  }
+
+  Int32OperandId guardToUint8Clamped(ValOperandId val) {
+    Int32OperandId res(nextOperandId_++);
+    writeOpWithOperandId(CacheOp::GuardToUint8Clamped, val);
+    writeOperandId(res);
+    return res;
+  }
+
   NumberOperandId guardIsNumber(ValOperandId val) {
     writeOpWithOperandId(CacheOp::GuardIsNumber, val);
     return NumberOperandId(val.id());
   }
 
   void guardType(ValOperandId val, ValueType type) {
     writeOpWithOperandId(CacheOp::GuardType, val);
     static_assert(sizeof(type) == sizeof(uint8_t),
@@ -1329,24 +1345,24 @@ class MOZ_RAII CacheIRWriter : public JS
 
   void storeDenseElement(ObjOperandId obj, Int32OperandId index,
                          ValOperandId rhs) {
     writeOpWithOperandId(CacheOp::StoreDenseElement, obj);
     writeOperandId(index);
     writeOperandId(rhs);
   }
 
-  void storeTypedElement(ObjOperandId obj, Int32OperandId index,
-                         ValOperandId rhs, TypedThingLayout layout,
-                         Scalar::Type elementType, bool handleOOB) {
+  void storeTypedElement(ObjOperandId obj, TypedThingLayout layout,
+                         Scalar::Type elementType, Int32OperandId index,
+                         OperandId rhs, bool handleOOB) {
     writeOpWithOperandId(CacheOp::StoreTypedElement, obj);
+    buffer_.writeByte(uint32_t(layout));
+    buffer_.writeByte(uint32_t(elementType));
     writeOperandId(index);
     writeOperandId(rhs);
-    buffer_.writeByte(uint32_t(layout));
-    buffer_.writeByte(uint32_t(elementType));
     buffer_.writeByte(uint32_t(handleOOB));
   }
 
   void storeDenseElementHole(ObjOperandId obj, Int32OperandId index,
                              ValOperandId rhs, bool handleAdd) {
     writeOpWithOperandId(CacheOp::StoreDenseElementHole, obj);
     writeOperandId(index);
     writeOperandId(rhs);
--- a/js/src/jit/IonCacheIRCompiler.cpp
+++ b/js/src/jit/IonCacheIRCompiler.cpp
@@ -783,16 +783,52 @@ bool IonCacheIRCompiler::emitGuardSpecif
     return false;
   }
 
   masm.branchPtr(Assembler::NotEqual, sym, ImmGCPtr(expected),
                  failure->label());
   return true;
 }
 
+bool IonCacheIRCompiler::emitGuardToInt32ModUint32() {
+  JitSpew(JitSpew_Codegen, __FUNCTION__);
+  ConstantOrRegister val =
+      allocator.useConstantOrRegister(masm, reader.valOperandId());
+  Register output = allocator.defineRegister(masm, reader.int32OperandId());
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  FloatRegister maybeTempDouble = ic_->asSetPropertyIC()->maybeTempDouble();
+  MOZ_ASSERT(maybeTempDouble != InvalidFloatReg);
+
+  return masm.truncateConstantOrRegisterToInt32(cx_, val, maybeTempDouble,
+                                                output, failure->label());
+}
+
+bool IonCacheIRCompiler::emitGuardToUint8Clamped() {
+  JitSpew(JitSpew_Codegen, __FUNCTION__);
+  ConstantOrRegister val =
+      allocator.useConstantOrRegister(masm, reader.valOperandId());
+  Register output = allocator.defineRegister(masm, reader.int32OperandId());
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  FloatRegister maybeTempDouble = ic_->asSetPropertyIC()->maybeTempDouble();
+  MOZ_ASSERT(maybeTempDouble != InvalidFloatReg);
+
+  return masm.clampConstantOrRegisterToUint8(cx_, val, maybeTempDouble, output,
+                                             failure->label());
+}
+
 bool IonCacheIRCompiler::emitLoadValueResult() {
   MOZ_CRASH("Baseline-specific op");
 }
 
 bool IonCacheIRCompiler::emitLoadFixedSlotResult() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   AutoOutputRegister output(*this);
   Register obj = allocator.useRegister(masm, reader.objOperandId());
@@ -1807,22 +1843,46 @@ bool IonCacheIRCompiler::emitStoreDenseE
 bool IonCacheIRCompiler::emitArrayPush() {
   MOZ_ASSERT_UNREACHABLE("emitArrayPush not supported for IonCaches.");
   return false;
 }
 
 bool IonCacheIRCompiler::emitStoreTypedElement() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   Register obj = allocator.useRegister(masm, reader.objOperandId());
-  Register index = allocator.useRegister(masm, reader.int32OperandId());
-  ConstantOrRegister val =
-      allocator.useConstantOrRegister(masm, reader.valOperandId());
-
   TypedThingLayout layout = reader.typedThingLayout();
   Scalar::Type arrayType = reader.scalarType();
+  Register index = allocator.useRegister(masm, reader.int32OperandId());
+
+  Maybe<Register> valInt32;
+  Maybe<ConstantOrRegister> valFloat;
+  switch (arrayType) {
+    case Scalar::Int8:
+    case Scalar::Uint8:
+    case Scalar::Int16:
+    case Scalar::Uint16:
+    case Scalar::Int32:
+    case Scalar::Uint32:
+    case Scalar::Uint8Clamped:
+      valInt32.emplace(allocator.useRegister(masm, reader.int32OperandId()));
+      break;
+
+    case Scalar::Float32:
+    case Scalar::Float64:
+      valFloat.emplace(
+          allocator.useConstantOrRegister(masm, reader.numberOperandId()));
+      break;
+
+    case Scalar::BigInt64:
+    case Scalar::BigUint64:
+    case Scalar::MaxTypedArrayViewType:
+    case Scalar::Int64:
+      MOZ_CRASH("Unsupported TypedArray type");
+  }
+
   bool handleOOB = reader.readBool();
 
   AutoScratchRegister scratch1(allocator, masm);
   AutoScratchRegister scratch2(allocator, masm);
 
   FailurePath* failure;
   if (!addFailurePath(&failure)) {
     return false;
@@ -1843,41 +1903,29 @@ bool IonCacheIRCompiler::emitStoreTypedE
   FloatRegister maybeTempDouble = ic_->asSetPropertyIC()->maybeTempDouble();
   FloatRegister maybeTempFloat32 = ic_->asSetPropertyIC()->maybeTempFloat32();
   MOZ_ASSERT(maybeTempDouble != InvalidFloatReg);
   MOZ_ASSERT_IF(jit::hasUnaliasedDouble(), maybeTempFloat32 != InvalidFloatReg);
 
   if (arrayType == Scalar::Float32) {
     FloatRegister tempFloat =
         hasUnaliasedDouble() ? maybeTempFloat32 : maybeTempDouble;
-    if (!masm.convertConstantOrRegisterToFloat(cx_, val, tempFloat,
+    if (!masm.convertConstantOrRegisterToFloat(cx_, *valFloat, tempFloat,
                                                failure->label())) {
       return false;
     }
     masm.storeToTypedFloatArray(arrayType, tempFloat, dest);
   } else if (arrayType == Scalar::Float64) {
-    if (!masm.convertConstantOrRegisterToDouble(cx_, val, maybeTempDouble,
+    if (!masm.convertConstantOrRegisterToDouble(cx_, *valFloat, maybeTempDouble,
                                                 failure->label())) {
       return false;
     }
     masm.storeToTypedFloatArray(arrayType, maybeTempDouble, dest);
   } else {
-    Register valueToStore = scratch2;
-    if (arrayType == Scalar::Uint8Clamped) {
-      if (!masm.clampConstantOrRegisterToUint8(
-              cx_, val, maybeTempDouble, valueToStore, failure->label())) {
-        return false;
-      }
-    } else {
-      if (!masm.truncateConstantOrRegisterToInt32(
-              cx_, val, maybeTempDouble, valueToStore, failure->label())) {
-        return false;
-      }
-    }
-    masm.storeToTypedIntArray(arrayType, valueToStore, dest);
+    masm.storeToTypedIntArray(arrayType, *valInt32, dest);
   }
 
   masm.bind(&done);
   return true;
 }
 
 bool IonCacheIRCompiler::emitCallNativeSetter() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);