Bug 208048

Summary: Fix Masm Probe Trampoline for MIPS
Product: WebKit Reporter: Paulo Matos <pmatos>
Component: New BugsAssignee: Paulo Matos <pmatos>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 208439    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Paulo Matos
Reported 2020-02-21 04:28:54 PST
Fix Masm Probe Trampoline for MIPS
Attachments
Patch (27.23 KB, patch)
2020-02-21 04:29 PST, Paulo Matos 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Paulo Matos
Comment 1 2020-02-21 04:29:12 PST
Created attachment 391393 [details] Patch
Paulo Matos
Comment 2 2020-02-21 04:36:21 PST
Currently MIPS32el is failing many tests with jit enabled. For a few reasons but one of them is due to the fact that ctiMasmProbeTrampoline implemented in MacroAssemblerMIPS.cpp is broken. This patch attempts to fix that. The reason the patch is broken is because it does not follow the MIPS C abi which requires sp, at the time a function is called, to point to the function incoming arguments. Currently ctiMasmProbeTrampoline calls executeProbe with sp pointing to the state structure setup on the stack. The C code follows the abi and stores the incoming argument (the State *) itself into 0($sp), causing the state structure to be corrupted and the program to crash. For this to work, we need to create a clean offset on the stack for the incoming arguments of executeProbe - which is why I have introduced EXECUTE_PROBE_ARGS_OFFSET which is set to (1 * PTR_SIZE) because executeProbe takes one argument. Unfortunately the patch cannot simply change the offsets by this value because it will cause all asserts to fail since they expect that the offsets to sp are the same as the offsets to *state, which are not (in the fixed version). Although the patch works, it can be improved which is why it is not r? yet.
Paulo Matos
Comment 3 2020-02-21 04:56:20 PST
I just noticed that unfortunately testmasm fails for mips still, which means there might still be some breakages to fix in this area of code. If you have a qemu-user-mipsel handy with a mipsel cross-cc try this (cross compiler and qemu can be easily be built off buildroot ci20 defconfig): /home/pmatos/roots/br-root.jsc32-mips/host/bin/qemu-mipsel -L /home/pmatos/roots/br-root.jsc32-mips/target ./WebKitBuild/Debug/bin/testmasm testBranchTruncateDoubleToInt32(42, 42)... testBranchTruncateDoubleToInt32(0, 0)... testGetEffectiveAddress(0xff00, 42, 8, CCallHelpers::TimesEight)... testBranchTruncateDoubleToInt32(42.7, 42)... testSimple()... testGetEffectiveAddress(0xff00, -200, -300, CCallHelpers::TimesEight)... testBranchTruncateDoubleToInt32(-1234, -1234)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::quiet_NaN(), 0)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::signaling_NaN(), 0)... testBranchTruncateDoubleToInt32(-std::numeric_limits<double>::infinity(), 0)... testBranchTruncateDoubleToInt32(-1234.56, -1234)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::infinity(), 0)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::max(), 0)... testBranchTruncateDoubleToInt32(-std::numeric_limits<double>::max(), 0)... testBranchTruncateDoubleToInt32(123, 123)... testCompareDouble(MacroAssembler::DoubleEqual)... testGetEffectiveAddress(0xff00, -200, -300, CCallHelpers::TimesEight): OK! testCompareDouble(MacroAssembler::DoubleNotEqual)... testGetEffectiveAddress(0xff00, 42, 8, CCallHelpers::TimesEight): OK! testCompareDouble(MacroAssembler::DoubleGreaterThan)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::signaling_NaN(), 0): OK! testCompareDouble(MacroAssembler::DoubleGreaterThanOrEqual)... testBranchTruncateDoubleToInt32(-1234, -1234): OK! testCompareDouble(MacroAssembler::DoubleLessThan)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::max(), 0): OK! testCompareDouble(MacroAssembler::DoubleLessThanOrEqual)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::infinity(), 0): OK! testCompareDouble(MacroAssembler::DoubleEqualOrUnordered)... testBranchTruncateDoubleToInt32(-std::numeric_limits<double>::max(), 0): OK! testCompareDouble(MacroAssembler::DoubleNotEqualOrUnordered)... testBranchTruncateDoubleToInt32(0, 0): OK! testCompareDouble(MacroAssembler::DoubleGreaterThanOrUnordered)... testSimple(): OK! testCompareDouble(MacroAssembler::DoubleGreaterThanOrEqualOrUnordered)... testBranchTruncateDoubleToInt32(-std::numeric_limits<double>::infinity(), 0): OK! testCompareDouble(MacroAssembler::DoubleLessThanOrUnordered)... testBranchTruncateDoubleToInt32(42, 42): OK! testCompareDouble(MacroAssembler::DoubleLessThanOrEqualOrUnordered)... testBranchTruncateDoubleToInt32(std::numeric_limits<double>::quiet_NaN(), 0): OK! testMul32WithImmediates()... testBranchTruncateDoubleToInt32(123, 123): OK! testBranchTruncateDoubleToInt32(-1234.56, -1234): OK! testProbeReadsArgumentRegisters()... testProbeWritesArgumentRegisters()... testBranchTruncateDoubleToInt32(42.7, 42): OK! testProbePreservesGPRS()... FAILED while testing cpu.gpr(GPRInfo::argumentGPR0): expected: 200208384, actual: 200208385 ASSERTION FAILED: CHECK_EQ(cpu.gpr(GPRInfo::argumentGPR0), testWord(0)) ../../Source/JavaScriptCore/assembler/testmasm.cpp(682) : {anonymous}::testProbeWritesArgumentRegisters()::<lambda(JSC::CCallHelpers&)>::<lambda(JSC::Probe::Context&)> qemu: uncaught target signal 6 (Aborted) - core dumped [1] 25153 abort /home/pmatos/roots/br-root.jsc32-mips/host/bin/qemu-mipsel -L
Paulo Matos
Comment 4 2020-03-02 06:02:04 PST
This has now been fixed and both ARMv7 and MIPSel are green with JIT enabled since r257466 (bug 208196). Closing this.
Note You need to log in before you can comment on or make changes to this bug.