commit 496027b477b663b9bbc2140b34dadfc4d6b1d059
parent fe186136bc53bc78d702235d16dfe598b015507e
Author: Ben Visness <bvisness@mozilla.com>
Date: Wed, 8 Oct 2025 19:42:40 +0000
Bug 1990354: Correctly swap bounds check condition. r=rhunt
Many wrongs were making a right here. We were incorrectly calling
InvertCondition instead of InvertCmpCondition, which meant that we had
off-by-one errors in 64-bit bounds checks. These erroneously-passing
bounds checks were nonetheless being caught by a second set of bounds
checks that we erroneously included due to the lack of an early return.
(In the memory case, the erroneously-passing bounds checks were being
caught by guard pages.)
Furthermore, InvertCmpOperands was itself wrong in many cases, turning
signed <= into > and so on. This has been wrong for a decade but has not
caused problems because nobody uses it and it only affects signed
comparisons. The loongarch64 folks also seem to have gotten this wrong
thanks to the way they adapted our code; we can only hope they have been
unaffected as well.
Differential Revision: https://phabricator.services.mozilla.com/D266020
Diffstat:
7 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/js/src/jit-test/tests/wasm/bce-x64-ion-codegen.js b/js/src/jit-test/tests/wasm/bce-x64-ion-codegen.js
@@ -31,10 +31,10 @@ for ( let memType of memTypes ) {
drop
(i32.load (local.get 1))))`,
'f', `
-cmp %r.., %r..
+(movq 0x08\\(%r..\\), %r..\ncmp %r.., %r..|cmpq 0x08\\(%r..\\), %r..)
jnb 0x00000000000000..
-movl \\(%r15,%r..,1\\), %e..
-movl \\(%r15,%r..,1\\), %eax`,
+movl \\(%r..,%r..,1\\), %e..
+movl \\(%r..,%r..,1\\), %eax`,
{no_prefix:true});
// Make sure constant indices below the heap minimum do not require a bounds
@@ -60,4 +60,3 @@ movl \\(%r15,%r..,1\\), %eax`,
mov \\$0xFFFF, %eax
movl \\(%r15,%rax,1\\), %eax`);
}
-
diff --git a/js/src/jit-test/tests/wasm/bce-x86-ion-codegen.js b/js/src/jit-test/tests/wasm/bce-x86-ion-codegen.js
@@ -23,7 +23,7 @@ codegenTestX86_adhoc(
drop
(i32.load (local.get 1))))`,
'f', `
-cmp %e.., %e..
+(movl 0x04\\(%r..\\), %e..\ncmp %e.., %e..|cmpl 0x04\\(%r..\\), %e..)
jnb 0x00000000000000..
movl \\(%r..,%r..,1\\), %e..
movl \\(%r..,%r..,1\\), %eax`,
diff --git a/js/src/jit/Lowering.cpp b/js/src/jit/Lowering.cpp
@@ -6746,6 +6746,8 @@ void LIRGenerator::visitWasmBoundsCheck(MWasmBoundsCheck* ins) {
add(lir, ins);
}
}
+
+ return;
}
if (index->type() == MIRType::Int64) {
diff --git a/js/src/jit/arm64/MacroAssembler-arm64.cpp b/js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ -2130,7 +2130,7 @@ void MacroAssembler::wasmBoundsCheck64(Condition cond, Register64 index,
void MacroAssembler::wasmBoundsCheck64(Condition cond, Register64 index,
Address boundsCheckLimit, Label* label) {
- branchPtr(InvertCondition(cond), boundsCheckLimit, index.reg, label);
+ branchPtr(SwapCmpOperandsCondition(cond), boundsCheckLimit, index.reg, label);
if (JitOptions.spectreIndexMasking) {
csel(ARMRegister(index.reg, 64), vixl::xzr, ARMRegister(index.reg, 64),
cond);
diff --git a/js/src/jit/arm64/vixl/Assembler-vixl.h b/js/src/jit/arm64/vixl/Assembler-vixl.h
@@ -164,7 +164,7 @@ class Assembler : public MozBaseAssembler {
}
// This is chaging the condition codes for cmp a, b to the same codes for cmp b, a.
- static inline Condition InvertCmpCondition(Condition cond) {
+ static inline Condition SwapCmpOperandsCondition(Condition cond) {
// Conditions al and nv behave identically, as "always true". They can't be
// inverted, because there is no "always false" condition.
switch (cond) {
@@ -172,13 +172,13 @@ class Assembler : public MozBaseAssembler {
case ne:
return cond;
case gt:
- return le;
+ return lt;
case le:
- return gt;
+ return ge;
case ge:
- return lt;
+ return le;
case lt:
- return ge;
+ return gt;
case hi:
return lo;
case lo:
diff --git a/js/src/jit/loong64/Assembler-loong64.cpp b/js/src/jit/loong64/Assembler-loong64.cpp
@@ -232,7 +232,7 @@ AssemblerLOONG64::DoubleCondition AssemblerLOONG64::InvertCondition(
}
}
-AssemblerLOONG64::Condition AssemblerLOONG64::InvertCmpCondition(
+AssemblerLOONG64::Condition AssemblerLOONG64::SwapCmpOperandsCondition(
Condition cond) {
switch (cond) {
case Equal:
@@ -243,9 +243,9 @@ AssemblerLOONG64::Condition AssemblerLOONG64::InvertCmpCondition(
case LessThanOrEqual:
return GreaterThanOrEqual;
case GreaterThan:
- return LessThanOrEqual;
- case GreaterThanOrEqual:
return LessThan;
+ case GreaterThanOrEqual:
+ return LessThanOrEqual;
case Above:
return Below;
case AboveOrEqual:
diff --git a/js/src/jit/loong64/Assembler-loong64.h b/js/src/jit/loong64/Assembler-loong64.h
@@ -985,7 +985,7 @@ class AssemblerLOONG64 : public AssemblerShared {
static DoubleCondition InvertCondition(DoubleCondition cond);
// This is changing the condition codes for cmp a, b to the same codes for cmp
// b, a.
- static Condition InvertCmpCondition(Condition cond);
+ static Condition SwapCmpOperandsCondition(Condition cond);
// As opposed to x86/x64 version, the data relocation has to be executed
// before to recover the pointer, and not after.
