1870747 - Assertion failure: Integer input should be equal or higher than Lowerbound., at jit/VMFunctions.cpp:2911

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug stack

for (let i = 0 ; i < 19 ; ++i) {
  (function () {
    return (9 >> (4294967295 + true)) % -1;
  })();
}

Assertion failure: Integer input should be equal or higher than Lowerbound., at /home/skygentoo/trees/mozilla-central/js/src/jit/VMFunctions.cpp:2911
#01: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x2745e36]
#02: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27eb2f9]
#03: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27e8257]
#04: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27f0bab]
#05: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27f1170]
#06: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x2b2ea01]
#07: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae2fbc]
#08: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae626c]
#09: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae66da]
#10: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1c6c628]
#11: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>)[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1c6c808]
#12: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1a23037]
#13: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1a22127]
#14: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x19e20b8]
#15: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x19dbfd6]
Simulator hit BKPT.
Use ARM_SIM_DEBUGGER=1 to enter the builtin debugger.
Hit MOZ_CRASH(ARM simulator breakpoint) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm/Simulator-arm.cpp:2819
#01: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27ecb70]
#02: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27e8186]
#03: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27f0bab]
#04: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x27f1170]
#05: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x2b2ea01]
#06: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae2fbc]
#07: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae626c]
#08: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1ae66da]
#09: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1c6c628]
#10: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>)[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1c6c808]
#11: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1a23037]
#12: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x1a22127]
#13: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x19e20b8]
#14: ???[/home/skygentoo/shell-cache/js-dbg-32-armsim32-linux-x86_64-382081ff53ef/js-dbg-32-armsim32-linux-x86_64-382081ff53ef +0x19dbfd6]
Segmentation fault

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6e95716978fc
user:        Ryan Hunt
date:        Thu Nov 30 00:46:31 2023 +0000
summary:     Bug 1863794 - Generate ABIFunctionType code using build script. r=jandem

Run with --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis --arm-hwcap=vfp, compile with PKG_CONFIG_PATH=/usr/lib/x86_64-linux-gnu/pkgconfig AR=ar 'CXX="clang++ -msse2 -mfpmath=sse"' 'CC="clang -msse2 -mfpmath=sse"' sh ../configure --host=x86_64-pc-linux-gnu --target=i686-pc-linux --enable-simulator=arm --enable-debug --with-ccache --enable-debug-symbols --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 382081ff53ef.

Ryan, is bug 1863794 a likely regressor? Setting s-s just-in-case for now.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1863794

Comment 2

[Iain Ireland [:iain]]

I think this is just a bug in the arm32 simulator.

In an arm32 build with --arm-hwcap=vfp, we call __aeabi_idivmod to compute 9 % -1. The result is an int64_t containing -9 (9 / -1) in the low 32 bits, and 0 (9 % - 1) in the high 32 bits: 0x00000000fffffff7. We return to this code:

case js::jit::Args_General2: {
  auto target = reinterpret_cast<Prototype_General2>(external);
  intptr_t ret;
  ret = target(a0, a1);
  scratchVolatileRegisters((void*)target); 
  setCallResult(ret);
  break;
}

This stores the result in an intptr_t, which truncates to 0xfffffff7 because this is a 32-bit build. When we call setCallResult, we convert from intptr_t to int64_t by sign-extending, giving us 0xfffffffffffffff7. This sets r1 to -1 instead of 0, which we catch because of --ion-check-range-analysis.

Here's a reduced testcase:

// |jit-test| --fast-warmup; --no-threads; --ion-check-range-analysis; --arm-hwcap=vfp
function foo(x) { return x % -1; }

with ({}) {}
for (var i = 0; i < 1000; i++) {
  foo(i);
}

Ryan, is this a bug in the generated ABIFunctionType code?

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

I think the issue is the ABIFunctionType, not the generated simulator code. This is probably caused by the old simulator code [1] being loose with the definition of Args_General2 and using int64_t for the return type even though the return type for ABIType::General should be intptr_t.

The signature of the target function is: extern MOZ_EXPORT int64_t __aeabi_idivmod(int, int);. The key thing is that it's returning an int64_t, which means that the correct ABIFunctionType here should be Args_Int64_GeneralGeneral, not Args_General2 [2]. I updated the wasm use of this and missed JS uses this too [3].

The ABIFunctionType used by JS is implicitly constructed by masm through calls to callWithABI. I see we're using MoveOp::General here [4], which is why we end up in Args_General2. There doesn't appear to be MoveOp::Int64. Is the second part of the call to __aeabi_idivmod being ignored? If so I guess we'd need some way to say we expect the return to be an int64_t but we only need the first int32_t.

[1]

-          Prototype_General2 target =
-              reinterpret_cast<Prototype_General2>(external);
-          int64_t result = target(arg0, arg1);
-          // The ARM backend makes calls to __aeabi_idivmod and
-          // __aeabi_uidivmod assuming that the float registers are
-          // non-volatile as a performance optimization, so the float
-          // registers must not be scratch when calling these.
-          bool scratchFloat =
-              target != __aeabi_idivmod && target != __aeabi_uidivmod;
-          scratchVolatileRegisters(/* scratchFloat = */ scratchFloat);
-          setCallResult(result);
-          break;

[2] https://searchfox.org/mozilla-central/source/js/src/jit/ABIFunctionType.yaml#16
[3] https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBuiltins.cpp#1169
[4] https://searchfox.org/mozilla-central/source/js/src/jit/arm/CodeGenerator-arm.cpp#572-577

Comment 4

[Iain Ireland [:iain]]

The ABIFunctionType used by JS is implicitly constructed by masm through calls to callWithABI. I see we're using MoveOp::General here [4], which is why we end up in Args_General2. There doesn't appear to be MoveOp::Int64. Is the second part of the call to __aeabi_idivmod being ignored? If so I guess we'd need some way to say we expect the return to be an int64_t but we only need the first int32_t.

Yeah, the low half of __aeabi_idivmod is being ignored.

It's a little awkward that MoveOp is used both for this and for the MoveResolver. It looks like way back in the day there used to be a variety of different enums in the places where we use MoveOp, but they were unified a decade ago in this patch.

I was wondering if it would make sense to replace MoveOp in the ABI machinery with ABIArgType, which seems to support all the types we care about, but it looks like passABIArg uses the MoveResolver internally.

Unless I'm missing something, I don't think this is security sensitive. Should we open it up?

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

I agree this is not sec-sensitive, the actual codegen should be correct but the simulator is getting the call redirection incorrect due to an incorrect ABIArgType.

Comment 6

[Donal Meehan [:dmeehan]]

:willyelm, could this be triaged for severity/priority? Is this something that needs a fix for Fx122?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Donal Meehan [:dmeehan] from comment #6)

:willyelm, could this be triaged for severity/priority? Is this something that needs a fix for Fx122?

This only affects the JS shell ARM simulator we use for testing, so it doesn't affect Firefox users. We should still fix it to not break fuzzing though.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:willyelm, could you have a look please?

For more information, please visit BugBot documentation.

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1870747: Convert AbiArgType into an enum class r=rhunt!

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1870747: Use ABIType instead of MoveOp::Type for callWithABI r=rhunt!

Depends on D198104

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1870747: Use ABIType::Int64 for aeabi_idivmod r=rhunt!

Depends on D198105

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a40c148875f1
Convert AbiArgType into an enum class r=rhunt
https://hg.mozilla.org/integration/autoland/rev/8a1b27094514
Use ABIType instead of MoveOp::Type for callWithABI r=rhunt
https://hg.mozilla.org/integration/autoland/rev/be7423b15b7d
Use ABIType::Int64 for aeabi_idivmod r=rhunt

Comment 13

[Cristian Tuns]

Backed out for causing build bustages in CodeGenerator.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/jit/CodeGenerator.cpp:12493:14: error: no matching member function for call to 'callWithABI'

Comment 14

[Iain Ireland [:iain]]

This collided with bug 1864323, which adds new uses of callWithABI. I will rebase and reland when those patches make it to central.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba8150681252
Convert AbiArgType into an enum class r=rhunt
https://hg.mozilla.org/integration/autoland/rev/94240bde6a76
Use ABIType instead of MoveOp::Type for callWithABI r=rhunt
https://hg.mozilla.org/integration/autoland/rev/0901a9f3e7ad
Use ABIType::Int64 for aeabi_idivmod r=rhunt

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ba8150681252
https://hg.mozilla.org/mozilla-central/rev/94240bde6a76
https://hg.mozilla.org/mozilla-central/rev/0901a9f3e7ad