Bug 1528240 - Fix inconsistent return type use in BaseCompiler::emitInstanceCall. r=lhansen.
authorJulian Seward <jseward@acm.org>
Fri, 15 Feb 2019 19:18:35 +0100
changeset 459566 55806c7e28b769963342000ee736246225f2b19d
parent 459565 4739353088fcad81bc126deab56e8de0fad7a88a
child 459567 09ba3e3539e8cf0ac105df4fb62e2bceb8d05f27
push id111971
push userjseward@mozilla.com
push dateFri, 15 Feb 2019 22:01:54 +0000
treeherdermozilla-inbound@55806c7e28b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslhansen
bugs1528240
milestone67.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 1528240 - Fix inconsistent return type use in BaseCompiler::emitInstanceCall. r=lhansen. emitInstanceCall() generates code to call a SymbolicAddress:: builtin. One of the parameters to emitInstanceCall() is the ABI-level return type of the call. If that is non-void, the returned value is pushed on to the compiler's operand stack. There are some functions, for example Instance::memFill, which return a value (an ExprType::I32) which is an implementation artefact, and which we don't want to push on the operand stack. As it stands, in such cases we lie to emitInstanceCall() by telling it the return type is ExprType::Void, so as to stop it pushing the return value. This inconsistency is confusing, and more importantly it gets in the way of ongoing work to move towards having single-point-of-truth definitions of argument and result types for SymbolicAddress:: builtin functions. This patch: * gives emitInstanceCall a new boolean parameter, saying whether or not the return value is to be pushed * defaults that parameter to |true|, so most uses don't have to change * for the few emitInstanceCall cases where the return type is non-void but we don't want to push it, passes |false| as required * renames auxiliary function pushReturnedIfNonVoid to pushReturnValueOfCall, as that better describes its modified role
js/src/wasm/WasmBaselineCompile.cpp
--- a/js/src/wasm/WasmBaselineCompile.cpp
+++ b/js/src/wasm/WasmBaselineCompile.cpp
@@ -6527,23 +6527,25 @@ class BaseCompiler final : public BaseCo
     uint32_t bytecodeOffset = iter_.lastOpcodeOffset();
 
     // The `valueAddr` is a raw pointer to the cell within some GC object or
     // TLS area, and we guarantee that the GC will not run while the
     // postbarrier call is active, so push a uintptr_t value.
 #ifdef JS_64BIT
     pushI64(RegI64(Register64(valueAddr)));
     if (!emitInstanceCall(bytecodeOffset, SigPL_, ExprType::Void,
-                          SymbolicAddress::PostBarrier)) {
+                          SymbolicAddress::PostBarrier,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
 #else
     pushI32(RegI32(valueAddr));
     if (!emitInstanceCall(bytecodeOffset, SigPI_, ExprType::Void,
-                          SymbolicAddress::PostBarrier)) {
+                          SymbolicAddress::PostBarrier,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
 #endif
     return true;
   }
 
   MOZ_MUST_USE bool emitBarrieredStore(const Maybe<RegPtr>& object,
                                        RegPtr valueAddr, RegPtr value,
@@ -6753,17 +6755,17 @@ class BaseCompiler final : public BaseCo
   MOZ_MUST_USE bool emitSetOrTeeLocal(uint32_t slot);
 
   void endBlock(ExprType type);
   void endLoop(ExprType type);
   void endIfThen();
   void endIfThenElse(ExprType type);
 
   void doReturn(ExprType returnType, bool popStack);
-  void pushReturnedIfNonVoid(const FunctionCall& call, ExprType type);
+  void pushReturnValueOfCall(const FunctionCall& call, ExprType type);
 
   void emitCompareI32(Assembler::Condition compareOp, ValType compareType);
   void emitCompareI64(Assembler::Condition compareOp, ValType compareType);
   void emitCompareF32(Assembler::DoubleCondition compareOp,
                       ValType compareType);
   void emitCompareF64(Assembler::DoubleCondition compareOp,
                       ValType compareType);
   void emitCompareRef(Assembler::Condition compareOp, ValType compareType);
@@ -6871,17 +6873,18 @@ class BaseCompiler final : public BaseCo
   void emitConvertI64ToF64();
   void emitConvertU64ToF64();
 #endif
   void emitReinterpretI32AsF32();
   void emitReinterpretI64AsF64();
   void emitRound(RoundingMode roundingMode, ValType operandType);
   MOZ_MUST_USE bool emitInstanceCall(uint32_t lineOrBytecode,
                                      const MIRTypeVector& sig, ExprType retType,
-                                     SymbolicAddress builtin);
+                                     SymbolicAddress builtin,
+                                     bool pushReturnedValue=true);
   MOZ_MUST_USE bool emitGrowMemory();
   MOZ_MUST_USE bool emitCurrentMemory();
 
   MOZ_MUST_USE bool emitRefNull();
   void emitRefIsNull();
 
   MOZ_MUST_USE bool emitAtomicCmpXchg(ValType type, Scalar::Type viewType);
   MOZ_MUST_USE bool emitAtomicLoad(ValType type, Scalar::Type viewType);
@@ -8622,22 +8625,19 @@ bool BaseCompiler::emitCallArgs(const Va
   for (size_t i = 0; i < numArgs; ++i) {
     passArg(argTypes[i], peek(numArgs - 1 - i), baselineCall);
   }
 
   masm.loadWasmTlsRegFromFrame();
   return true;
 }
 
-void BaseCompiler::pushReturnedIfNonVoid(const FunctionCall& call,
+void BaseCompiler::pushReturnValueOfCall(const FunctionCall& call,
                                          ExprType type) {
   switch (type.code()) {
-    case ExprType::Void:
-      // There's no return value.  Do nothing.
-      break;
     case ExprType::I32: {
       RegI32 rv = captureReturnedI32();
       pushI32(rv);
       break;
     }
     case ExprType::I64: {
       RegI64 rv = captureReturnedI64();
       pushI64(rv);
@@ -8657,16 +8657,18 @@ void BaseCompiler::pushReturnedIfNonVoid
     case ExprType::AnyRef: {
       RegPtr rv = captureReturnedRef();
       pushRef(rv);
       break;
     }
     case ExprType::NullRef:
       MOZ_CRASH("NullRef not expressible");
     default:
+      // In particular, passing |type| == ExprType::Void to this function is
+      // an error.
       MOZ_CRASH("Function return type");
   }
 }
 
 // For now, always sync() at the beginning of the call to easily save live
 // values.
 //
 // TODO / OPTIMIZE (Bug 1316806): We may be able to avoid a full sync(), since
@@ -8719,17 +8721,19 @@ bool BaseCompiler::emitCall() {
   if (!createStackMap("emitCall", raOffset)) {
     return false;
   }
 
   endCall(baselineCall, stackSpace);
 
   popValueStackBy(numArgs);
 
-  pushReturnedIfNonVoid(baselineCall, funcType.ret());
+  if (funcType.ret() != ExprType::Void) {
+    pushReturnValueOfCall(baselineCall, funcType.ret());
+  }
 
   return true;
 }
 
 bool BaseCompiler::emitCallIndirect() {
   uint32_t lineOrBytecode = readCallSiteLineOrBytecode();
 
   uint32_t funcTypeIndex;
@@ -8771,17 +8775,19 @@ bool BaseCompiler::emitCallIndirect() {
   if (!createStackMap("emitCallIndirect", raOffset)) {
     return false;
   }
 
   endCall(baselineCall, stackSpace);
 
   popValueStackBy(numArgs);
 
-  pushReturnedIfNonVoid(baselineCall, funcType.ret());
+  if (funcType.ret() != ExprType::Void) {
+    pushReturnValueOfCall(baselineCall, funcType.ret());
+  }
 
   return true;
 }
 
 void BaseCompiler::emitRound(RoundingMode roundingMode, ValType operandType) {
   if (operandType == ValType::F32) {
     RegF32 f0 = popF32();
     roundF32(roundingMode, f0);
@@ -8834,17 +8840,18 @@ bool BaseCompiler::emitUnaryMathBuiltinC
   if (!createStackMap("emitUnaryMathBuiltin[..]", raOffset)) {
     return false;
   }
 
   endCall(baselineCall, stackSpace);
 
   popValueStackBy(numArgs);
 
-  pushReturnedIfNonVoid(baselineCall, retType);
+  // We know retType isn't ExprType::Void here, so there's no need to check it.
+  pushReturnValueOfCall(baselineCall, retType);
 
   return true;
 }
 
 #ifdef RABALDR_INT_DIV_I64_CALLOUT
 bool BaseCompiler::emitDivOrModI64BuiltinCall(SymbolicAddress callee,
                                               ValType operandType) {
   MOZ_ASSERT(operandType == ValType::I64);
@@ -9763,17 +9770,18 @@ void BaseCompiler::emitCompareRef(Assemb
   masm.cmpPtrSet(compareOp, rs1, rs2, rd);
   freeRef(rs1);
   freeRef(rs2);
   pushI32(rd);
 }
 
 bool BaseCompiler::emitInstanceCall(uint32_t lineOrBytecode,
                                     const MIRTypeVector& sig, ExprType retType,
-                                    SymbolicAddress builtin) {
+                                    SymbolicAddress builtin,
+                                    bool pushReturnedValue/*=true*/) {
   MOZ_ASSERT(sig[0] == MIRType::Pointer);
 
   sync();
 
   uint32_t numArgs = sig.length() - 1 /* instance */;
   size_t stackSpace = stackConsumed(numArgs);
 
   FunctionCall baselineCall(lineOrBytecode);
@@ -9814,17 +9822,20 @@ bool BaseCompiler::emitInstanceCall(uint
   //
   // Furthermore, clients assume that even if retType == ExprType::Void, the
   // callee may have returned a status result and left it in ReturnReg for us
   // to find, and that that register will not be destroyed here (or above).
   // In this case the callee will have a C++ declaration stating that there is
   // a return value.  Examples include memory and table operations that are
   // implemented as callouts.
 
-  pushReturnedIfNonVoid(baselineCall, retType);
+  if (pushReturnedValue) {
+    MOZ_ASSERT(retType != ExprType::Void);
+    pushReturnValueOfCall(baselineCall, retType);
+  }
   return true;
 }
 
 bool BaseCompiler::emitGrowMemory() {
   uint32_t lineOrBytecode = readCallSiteLineOrBytecode();
 
   Nothing arg;
   if (!iter_.readGrowMemory(&arg)) {
@@ -10230,25 +10241,27 @@ bool BaseCompiler::emitMemOrTableCopy(bo
   if (deadCode_) {
     return true;
   }
 
   // Returns -1 on trap, otherwise 0.
   if (isMem) {
     MOZ_ASSERT(srcMemOrTableIndex == 0);
     MOZ_ASSERT(dstMemOrTableIndex == 0);
-    if (!emitInstanceCall(lineOrBytecode, SigPIII_, ExprType::Void,
-                          SymbolicAddress::MemCopy)) {
+    if (!emitInstanceCall(lineOrBytecode, SigPIII_, ExprType::I32,
+                          SymbolicAddress::MemCopy,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
   } else {
     pushI32(dstMemOrTableIndex);
     pushI32(srcMemOrTableIndex);
-    if (!emitInstanceCall(lineOrBytecode, SigPIIIII_, ExprType::Void,
-                          SymbolicAddress::TableCopy)) {
+    if (!emitInstanceCall(lineOrBytecode, SigPIIIII_, ExprType::I32,
+                          SymbolicAddress::TableCopy,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
   }
 
   Label ok;
   masm.branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &ok);
   trap(Trap::ThrowReported);
   masm.bind(&ok);
@@ -10269,17 +10282,18 @@ bool BaseCompiler::emitDataOrElemDrop(bo
   }
 
   // Despite the cast to int32_t, the callee regards the value as unsigned.
   //
   // Returns -1 on trap, otherwise 0.
   pushI32(int32_t(segIndex));
   SymbolicAddress callee =
       isData ? SymbolicAddress::DataDrop : SymbolicAddress::ElemDrop;
-  if (!emitInstanceCall(lineOrBytecode, SigPI_, ExprType::Void, callee)) {
+  if (!emitInstanceCall(lineOrBytecode, SigPI_, ExprType::Void, callee,
+                        /*pushReturnedValue=*/false)) {
     return false;
   }
 
   Label ok;
   masm.branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &ok);
   trap(Trap::ThrowReported);
   masm.bind(&ok);
 
@@ -10295,17 +10309,18 @@ bool BaseCompiler::emitMemFill() {
   }
 
   if (deadCode_) {
     return true;
   }
 
   // Returns -1 on trap, otherwise 0.
   if (!emitInstanceCall(lineOrBytecode, SigPIII_, ExprType::Void,
-                        SymbolicAddress::MemFill)) {
+                        SymbolicAddress::MemFill,
+                        /*pushReturnedValue=*/false)) {
     return false;
   }
 
   Label ok;
   masm.branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &ok);
   trap(Trap::ThrowReported);
   masm.bind(&ok);
 
@@ -10326,23 +10341,25 @@ bool BaseCompiler::emitMemOrTableInit(bo
   if (deadCode_) {
     return true;
   }
 
   // Returns -1 on trap, otherwise 0.
   pushI32(int32_t(segIndex));
   if (isMem) {
     if (!emitInstanceCall(lineOrBytecode, SigPIIII_, ExprType::Void,
-                          SymbolicAddress::MemInit)) {
+                          SymbolicAddress::MemInit,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
   } else {
     pushI32(dstTableIndex);
     if (!emitInstanceCall(lineOrBytecode, SigPIIIII_, ExprType::Void,
-                          SymbolicAddress::TableInit)) {
+                          SymbolicAddress::TableInit,
+                          /*pushReturnedValue=*/false)) {
       return false;
     }
   }
 
   Label ok;
   masm.branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &ok);
   trap(Trap::ThrowReported);
   masm.bind(&ok);
@@ -10410,18 +10427,19 @@ bool BaseCompiler::emitTableSet() {
   }
   if (deadCode_) {
     return true;
   }
   // set(index:u32, value:ref, table:u32) -> i32
   //
   // Returns -1 on range error, otherwise 0 (which is then ignored).
   pushI32(tableIndex);
-  if (!emitInstanceCall(lineOrBytecode, SigPIRI_, ExprType::Void,
-                        SymbolicAddress::TableSet)) {
+  if (!emitInstanceCall(lineOrBytecode, SigPIRI_, ExprType::I32,
+                        SymbolicAddress::TableSet,
+                        /*pushReturnedValue=*/false)) {
     return false;
   }
   Label noTrap;
   masm.branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &noTrap);
   trap(Trap::ThrowReported);
   masm.bind(&noTrap);
   return true;
 }