- Revision
- 289354
- Author
- sbar...@apple.com
- Date
- 2022-02-07 19:00:28 -0800 (Mon, 07 Feb 2022)
Log Message
Wasm crash on https://copy.sh/v86/?profile=""
https://bugs.webkit.org/show_bug.cgi?id=236037
rdar://88358719
Reviewed by Mark Lam.
Lower stack args in Air had a bug where it was emitting a constant
materialization at the wrong instruction offset for certain types
of spill instructions. This happens when we have a stack slot that
is 8 bytes wide, but we're emitting a zero def Move32. We need to
zero the upper 4 bytes. However, there is also code inside lower
stack args that uses the temp register when encountering offsets
that are too large to encode in a single instruction. However,
this offset materialization code for the second Move32 to zero
the upper bytes was happening before the actual store. For example,
we'd end up with:
movz x16, #k
movz x16, #k2
stur x1, [x16]
stur zr, [x16]
instead of
movz x16, #k
stur x1, [x16]
movz x16, #k2
stur zr, [x16]
* b3/air/AirLowerStackArgs.cpp:
(JSC::B3::Air::lowerStackArgs):
* b3/air/testair.cpp:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (289353 => 289354)
--- trunk/Source/_javascript_Core/ChangeLog 2022-02-08 02:58:30 UTC (rev 289353)
+++ trunk/Source/_javascript_Core/ChangeLog 2022-02-08 03:00:28 UTC (rev 289354)
@@ -1,3 +1,36 @@
+2022-02-07 Saam Barati <sbar...@apple.com>
+
+ Wasm crash on https://copy.sh/v86/?profile=""
+ https://bugs.webkit.org/show_bug.cgi?id=236037
+ rdar://88358719
+
+ Reviewed by Mark Lam.
+
+ Lower stack args in Air had a bug where it was emitting a constant
+ materialization at the wrong instruction offset for certain types
+ of spill instructions. This happens when we have a stack slot that
+ is 8 bytes wide, but we're emitting a zero def Move32. We need to
+ zero the upper 4 bytes. However, there is also code inside lower
+ stack args that uses the temp register when encountering offsets
+ that are too large to encode in a single instruction. However,
+ this offset materialization code for the second Move32 to zero
+ the upper bytes was happening before the actual store. For example,
+ we'd end up with:
+ movz x16, #k
+ movz x16, #k2
+ stur x1, [x16]
+ stur zr, [x16]
+
+ instead of
+ movz x16, #k
+ stur x1, [x16]
+ movz x16, #k2
+ stur zr, [x16]
+
+ * b3/air/AirLowerStackArgs.cpp:
+ (JSC::B3::Air::lowerStackArgs):
+ * b3/air/testair.cpp:
+
2022-02-06 Lauro Moura <lmo...@igalia.com>
Unreviewed, non-unified build fixes
Modified: trunk/Source/_javascript_Core/b3/air/AirLowerStackArgs.cpp (289353 => 289354)
--- trunk/Source/_javascript_Core/b3/air/AirLowerStackArgs.cpp 2022-02-08 02:58:30 UTC (rev 289353)
+++ trunk/Source/_javascript_Core/b3/air/AirLowerStackArgs.cpp 2022-02-08 03:00:28 UTC (rev 289354)
@@ -110,7 +110,7 @@
inst.forEachArg(
[&] (Arg& arg, Arg::Role role, Bank, Width width) {
- auto stackAddr = [&] (Value::OffsetType offsetFromFP) -> Arg {
+ auto stackAddr = [&] (unsigned instIndex, Value::OffsetType offsetFromFP) -> Arg {
int32_t offsetFromSP = offsetFromFP + code.frameSize();
if (inst.admitsExtendedOffsetAddr(arg)) {
@@ -137,6 +137,7 @@
result = Arg::addr(tmp, 0);
return result;
#elif CPU(X86_64)
+ UNUSED_PARAM(instIndex);
// Can't happen on x86: immediates are always big enough for frame size.
RELEASE_ASSERT_NOT_REACHED();
#else
@@ -171,13 +172,13 @@
RELEASE_ASSERT(isValidForm(storeOpcode, operandKind, Arg::Stack));
insertionSet.insert(
instIndex + 1, storeOpcode, inst.origin, operand,
- stackAddr(arg.offset() + 4 + slot->offsetFromFP()));
+ stackAddr(instIndex + 1, arg.offset() + 4 + slot->offsetFromFP()));
}
- arg = stackAddr(arg.offset() + slot->offsetFromFP());
+ arg = stackAddr(instIndex, arg.offset() + slot->offsetFromFP());
break;
}
case Arg::CallArg:
- arg = stackAddr(arg.offset() - code.frameSize());
+ arg = stackAddr(instIndex, arg.offset() - code.frameSize());
break;
default:
break;
Modified: trunk/Source/_javascript_Core/b3/air/testair.cpp (289353 => 289354)
--- trunk/Source/_javascript_Core/b3/air/testair.cpp 2022-02-08 02:58:30 UTC (rev 289353)
+++ trunk/Source/_javascript_Core/b3/air/testair.cpp 2022-02-08 03:00:28 UTC (rev 289354)
@@ -2360,6 +2360,37 @@
CHECK(runResult == 99);
}
+void testZDefOfSpillSlotWithOffsetNeedingToBeMaterializedInARegister()
+{
+ if (Options::defaultB3OptLevel() == 2)
+ return;
+
+ B3::Procedure proc;
+ Code& code = proc.code();
+
+ BasicBlock* root = code.addBlock();
+
+ Vector<StackSlot*> slots;
+ for (unsigned i = 0; i < 10000; ++i)
+ slots.append(code.addStackSlot(8, StackSlotKind::Spill));
+
+ for (auto* slot : slots)
+ root->append(Move32, nullptr, Tmp(GPRInfo::argumentGPR0), Arg::stack(slot));
+
+ Tmp loadResult = code.newTmp(GP);
+ Tmp sum = code.newTmp(GP);
+ root->append(Move, nullptr, Arg::imm(0), sum);
+ for (auto* slot : slots) {
+ root->append(Move, nullptr, Arg::stack(slot), loadResult);
+ root->append(Add64, nullptr, loadResult, sum);
+ }
+ root->append(Move, nullptr, sum, Tmp(GPRInfo::returnValueGPR));
+ root->append(Ret64, nullptr, Tmp(GPRInfo::returnValueGPR));
+
+ int32_t result = compileAndRun<int>(proc, 1);
+ CHECK(result == 10000);
+}
+
#define PREFIX "O", Options::defaultB3OptLevel(), ": "
#define RUN(test) do { \
@@ -2455,6 +2486,8 @@
RUN(testLinearScanSpillRangesLateUse());
RUN(testLinearScanSpillRangesEarlyDef());
+ RUN(testZDefOfSpillSlotWithOffsetNeedingToBeMaterializedInARegister());
+
if (tasks.isEmpty())
usage();
