Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (97330 => 97331)
--- trunk/Source/_javascript_Core/ChangeLog 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/ChangeLog 2011-10-13 01:44:14 UTC (rev 97331)
@@ -1,5 +1,48 @@
2011-10-12 Gavin Barraclough <[email protected]>
+ MacroAssemblerX86 8-bit register ops unsafe on CPU(X86)
+ https://bugs.webkit.org/show_bug.cgi?id=69978
+
+ Reviewed by Filip Pizlo.
+
+ Certain ops are unsafe if the register passed is esp..edi (will instead test/set the ).
+
+ compare32/test8/test32 Call setCC, which sets an 8-bit register - we can fix this by adding
+ a couple of xchg instructions.
+
+ branchTest8 with a register argument is also affected. In all cases this is currently used
+ this is testing a value that is correct to 32 or more bits, so we can simply switch these
+ to branchTest32 & remove the corresponding branchTest8 (this is desirable anyway, since the
+ 32-bit form is cheaper to implement on platforms that don't have an 8-bit compare instruction).
+
+ This fixes the remaining fast/js failures with the DFG JIT 32_64.
+
+ * assembler/MacroAssemblerARMv7.h
+ - removed branchTest8.
+ * assembler/MacroAssemblerX86Common.h:
+ (JSC::MacroAssemblerX86Common::compare32):
+ (JSC::MacroAssemblerX86Common::test8):
+ (JSC::MacroAssemblerX86Common::test32):
+ (JSC::MacroAssemblerX86Common::set32):
+ - added set32 helper that is 'h' register safe.
+ - removed branchTest8.
+ * dfg/DFGJITCodeGenerator32_64.cpp:
+ (JSC::DFG::JITCodeGenerator::nonSpeculativePeepholeBranch):
+ (JSC::DFG::JITCodeGenerator::nonSpeculativePeepholeStrictEq):
+ - switch uses of branchTest8 to branchTest32.
+ * dfg/DFGJITCodeGenerator64.cpp:
+ (JSC::DFG::JITCodeGenerator::nonSpeculativePeepholeBranch):
+ (JSC::DFG::JITCodeGenerator::nonSpeculativePeepholeStrictEq):
+ - switch uses of branchTest8 to branchTest32.
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::emitBranch):
+ - switch uses of branchTest8 to branchTest32.
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::emitBranch):
+ - switch uses of branchTest8 to branchTest32.
+
+2011-10-12 Gavin Barraclough <[email protected]>
+
Errrk, revert accidental commit!
* wtf/Platform.h:
Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h (97330 => 97331)
--- trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h 2011-10-13 01:44:14 UTC (rev 97331)
@@ -1013,12 +1013,6 @@
return branchTest32(cond, addressTempRegister, mask);
}
- Jump branchTest8(ResultCondition cond, RegisterID reg, TrustedImm32 mask = TrustedImm32(-1))
- {
- test32(reg, mask);
- return Jump(makeBranch(cond));
- }
-
Jump branchTest8(ResultCondition cond, Address address, TrustedImm32 mask = TrustedImm32(-1))
{
// use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/
Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h (97330 => 97331)
--- trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h 2011-10-13 01:44:14 UTC (rev 97331)
@@ -931,17 +931,6 @@
return Jump(m_assembler.jCC(x86Condition(cond)));
}
- Jump branchTest8(ResultCondition cond, RegisterID reg, TrustedImm32 mask = TrustedImm32(-1))
- {
- // Byte in TrustedImm32 is not well defined, so be a little permisive here, but don't accept nonsense values.
- ASSERT(mask.m_value >= -128 && mask.m_value <= 255);
- if (mask.m_value == -1)
- m_assembler.testb_rr(reg, reg);
- else
- m_assembler.testb_i8r(mask.m_value, reg);
- return Jump(m_assembler.jCC(x86Condition(cond)));
- }
-
Jump branchTest8(ResultCondition cond, Address address, TrustedImm32 mask = TrustedImm32(-1))
{
// Byte in TrustedImm32 is not well defined, so be a little permisive here, but don't accept nonsense values.
@@ -1163,8 +1152,7 @@
void compare32(RelationalCondition cond, RegisterID left, RegisterID right, RegisterID dest)
{
m_assembler.cmpl_rr(right, left);
- m_assembler.setCC_r(x86Condition(cond), dest);
- m_assembler.movzbl_rr(dest, dest);
+ set32(x86Condition(cond), dest);
}
void compare32(RelationalCondition cond, RegisterID left, TrustedImm32 right, RegisterID dest)
@@ -1173,12 +1161,11 @@
m_assembler.testl_rr(left, left);
else
m_assembler.cmpl_ir(right.m_value, left);
- m_assembler.setCC_r(x86Condition(cond), dest);
- m_assembler.movzbl_rr(dest, dest);
+ set32(x86Condition(cond), dest);
}
// FIXME:
- // The mask should be optional... paerhaps the argument order should be
+ // The mask should be optional... perhaps the argument order should be
// dest-src, operations always have a dest? ... possibly not true, considering
// asm ops like test, or pseudo ops like pop().
@@ -1188,8 +1175,7 @@
m_assembler.cmpb_im(0, address.offset, address.base);
else
m_assembler.testb_im(mask.m_value, address.offset, address.base);
- m_assembler.setCC_r(x86Condition(cond), dest);
- m_assembler.movzbl_rr(dest, dest);
+ set32(x86Condition(cond), dest);
}
void test32(ResultCondition cond, Address address, TrustedImm32 mask, RegisterID dest)
@@ -1198,8 +1184,7 @@
m_assembler.cmpl_im(0, address.offset, address.base);
else
m_assembler.testl_i32m(mask.m_value, address.offset, address.base);
- m_assembler.setCC_r(x86Condition(cond), dest);
- m_assembler.movzbl_rr(dest, dest);
+ set32(x86Condition(cond), dest);
}
// Invert a relational condition, e.g. == becomes !=, < becomes >=, etc.
@@ -1224,6 +1209,23 @@
return static_cast<X86Assembler::Condition>(cond);
}
+ void set32(X86Assembler::Condition cond, RegisterID dest)
+ {
+#if CPU(X86)
+ // On 32-bit x86 we can only set the first 4 registers;
+ // esp..edi are mapped to the 'h' registers!
+ if (dest >= 4) {
+ m_assembler.xchgl_rr(dest, X86Registers::eax);
+ m_assembler.setCC_r(cond, X86Registers::eax);
+ m_assembler.movzbl_rr(X86Registers::eax, X86Registers::eax);
+ m_assembler.xchgl_rr(dest, X86Registers::eax);
+ return;
+ }
+#endif
+ m_assembler.setCC_r(cond, dest);
+ m_assembler.movzbl_rr(dest, dest);
+ }
+
private:
// Only MacroAssemblerX86 should be using the following method; SSE2 is always available on
// x86_64, and clients & subclasses of MacroAssembler should be using 'supportsFloatingPoint()'.
Modified: trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator32_64.cpp (97330 => 97331)
--- trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator32_64.cpp 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator32_64.cpp 2011-10-13 01:44:14 UTC (rev 97331)
@@ -1062,7 +1062,7 @@
flushRegisters();
callOperation(helperFunction, resultGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);
- addBranch(m_jit.branchTest8(callResultCondition, resultGPR), taken);
+ addBranch(m_jit.branchTest32(callResultCondition, resultGPR), taken);
} else {
GPRTemporary result(this);
GPRReg resultGPR = result.gpr();
@@ -1086,7 +1086,7 @@
callOperation(helperFunction, resultGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(callResultCondition, resultGPR), taken);
+ addBranch(m_jit.branchTest32(callResultCondition, resultGPR), taken);
}
}
@@ -1191,7 +1191,7 @@
callOperation(operationCompareStrictEqCell, resultPayloadGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);
silentFillAllRegisters(resultPayloadGPR);
- addBranch(m_jit.branchTest8(invert ? JITCompiler::NonZero : JITCompiler::Zero, resultPayloadGPR), taken);
+ addBranch(m_jit.branchTest32(invert ? JITCompiler::NonZero : JITCompiler::Zero, resultPayloadGPR), taken);
} else {
// FIXME: Add fast paths for twoCells, number etc.
@@ -1199,7 +1199,7 @@
callOperation(operationCompareStrictEq, resultPayloadGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);
silentFillAllRegisters(resultPayloadGPR);
- addBranch(m_jit.branchTest8(invert ? JITCompiler::Zero : JITCompiler::NonZero, resultPayloadGPR), taken);
+ addBranch(m_jit.branchTest32(invert ? JITCompiler::Zero : JITCompiler::NonZero, resultPayloadGPR), taken);
}
if (notTaken != (m_block + 1))
Modified: trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator64.cpp (97330 => 97331)
--- trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator64.cpp 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/dfg/DFGJITCodeGenerator64.cpp 2011-10-13 01:44:14 UTC (rev 97331)
@@ -1020,7 +1020,7 @@
flushRegisters();
callOperation(helperFunction, resultGPR, arg1GPR, arg2GPR);
- addBranch(m_jit.branchTest8(callResultCondition, resultGPR), taken);
+ addBranch(m_jit.branchTest32(callResultCondition, resultGPR), taken);
} else {
GPRTemporary result(this, arg2);
GPRReg resultGPR = result.gpr();
@@ -1044,7 +1044,7 @@
callOperation(helperFunction, resultGPR, arg1GPR, arg2GPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(callResultCondition, resultGPR), taken);
+ addBranch(m_jit.branchTest32(callResultCondition, resultGPR), taken);
}
}
@@ -1142,7 +1142,7 @@
callOperation(operationCompareStrictEqCell, resultGPR, arg1GPR, arg2GPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(invert ? JITCompiler::NonZero : JITCompiler::Zero, resultGPR), taken);
+ addBranch(m_jit.branchTest32(invert ? JITCompiler::NonZero : JITCompiler::Zero, resultGPR), taken);
} else {
m_jit.orPtr(arg1GPR, arg2GPR, resultGPR);
@@ -1162,7 +1162,7 @@
callOperation(operationCompareStrictEq, resultGPR, arg1GPR, arg2GPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(invert ? JITCompiler::Zero : JITCompiler::NonZero, resultGPR), taken);
+ addBranch(m_jit.branchTest32(invert ? JITCompiler::Zero : JITCompiler::NonZero, resultGPR), taken);
}
if (notTaken != (m_block + 1))
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (97330 => 97331)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2011-10-13 01:44:14 UTC (rev 97331)
@@ -644,7 +644,7 @@
callOperation(dfgConvertJSValueToBoolean, resultGPR, valueTagGPR, valuePayloadGPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(JITCompiler::NonZero, resultGPR), taken);
+ addBranch(m_jit.branchTest32(JITCompiler::NonZero, resultGPR), taken);
if (notTaken != (m_block + 1))
addBranch(m_jit.jump(), notTaken);
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (97330 => 97331)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2011-10-13 01:33:57 UTC (rev 97330)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2011-10-13 01:44:14 UTC (rev 97331)
@@ -773,7 +773,7 @@
callOperation(dfgConvertJSValueToBoolean, resultGPR, valueGPR);
silentFillAllRegisters(resultGPR);
- addBranch(m_jit.branchTest8(MacroAssembler::NonZero, resultGPR), taken);
+ addBranch(m_jit.branchTest32(MacroAssembler::NonZero, resultGPR), taken);
if (notTaken != (m_block + 1))
addBranch(m_jit.jump(), notTaken);
}
