Wrong code generation of i64.mul in MIPS and Loong64 Ion compilers
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Tracking
()
People
(Reporter: parkjuny, Assigned: zhaojiazhong-hf)
References
(Blocks 1 open bug)
Details
(Keywords: reporter-external, sec-audit, Whiteboard: [adv-main116-][adv-ESR115.1-])
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
Overview
Target: SpiderMonkey MIPS64, MIPS32, Loong64 5/31 version - commit 8dd0d2bebe4c897152da6c86d937e4be80bbaa54
Wrong code generation of i64.mul in MIPS and Loong64 Ion compilers result in wrong optimization results.
That is, multiplication of i64 with 2^n +- 1 wrongly results in 0 due to wrong code generation and register allocation.
Proof of Concept Code
(module
(type (;0;) (func (param i64) (result i64)))
(import "mem" "mem" (memory (;0;) 1024))
(func (;0;) (type 0) (param i64) (result i64)
local.get 0
i64.const 4095
i64.mul)
(export "main" (func 0)))
const code = new Uint8Array([0,97,115,109,1,0,0,0,1,6,1,96,1,126,1,126,2,13,1,3,109,101,109,3,109,101,109,2,0,128,8,3,2,1,0,7,8,1,4,109,97,105,110,0,0,10,10,1,8,0,32,0,66,255,31,126,11,11,1,0]);
let memory = new WebAssembly.Memory({
initial: 1024
})
const module = new WebAssembly.Module(code);
const instance = new WebAssembly.Instance(module, {
mem: {
mem: memory
}
});
print(instance.exports.main(42n));
$ builds/loong64_debug/dist/bin/js --wasm-compiler=ion poc.js
0
$ builds/mips64_debug/dist/bin/js --wasm-compiler=ion poc.js
0
$ builds/mips32_debug/dist/bin/js --wasm-compiler=ion poc.js
0
Correct value is 171990, which is $42 \times 4095$
Root Cause Analysis
When generating code from LMulI64
LIR which contains constant 2^n +-1 as an operand, output register (from ToOutRegister64
) and the temporary register used as the operand of add64
and sub64
(ToRegister64
) can be allocated to the same register. This can result in wrong code generation that the result becomes output + output
for add64
(2^n + 1 case), and output - output = 0
for sub64
(2^n - 1 case).
// js/src/jit/loong64/CodeGenerator-loong64.cpp:1071
void CodeGenerator::visitMulI64(LMulI64* lir) {
const LInt64Allocation lhs = lir->getInt64Operand(LMulI64::Lhs);
const LInt64Allocation rhs = lir->getInt64Operand(LMulI64::Rhs);
const Register64 output = ToOutRegister64(lir);
if (IsConstant(rhs)) {
int64_t constant = ToInt64(rhs);
switch (constant) {
case -1:
// (...)
case 0:
// (...)
case 1:
// nop
return;
default:
if (constant > 0) {
if (mozilla::IsPowerOfTwo(static_cast<uint32_t>(constant + 1))) {
masm.move64(ToRegister64(lhs), output);
masm.lshift64(Imm32(FloorLog2(constant + 1)), output);
masm.sub64(ToRegister64(lhs), output);
return;
} else if (mozilla::IsPowerOfTwo(
static_cast<uint32_t>(constant - 1))) {
masm.move64(ToRegister64(lhs), output);
masm.lshift64(Imm32(FloorLog2(constant - 1u)), output);
masm.add64(ToRegister64(lhs), output);
return;
}
// Use shift if constant is power of 2.
// (...)
}
// (...)
}
} else {
// (...)
}
}
For an example, let's consider an example program that takes an i64 as an argument and returns the multiplication with 4095 (2^12 - 1). Then, the following program is compiled as below:
(module
(type (;0;) (func (param i64) (result i64)))
(import "mem" "mem" (memory (;0;) 1024))
(func (;0;) (type 0) (param i64) (result i64)
local.get 0
i64.const 4095
i64.mul)
(export "main" (func 0)))
0000000000000000 <.data>:
0: 02ffe063 addi.d $sp, $sp, -8(0xff8)
4: 29c00061 st.d $ra, $sp, 0
8: 02ffe063 addi.d $sp, $sp, -8(0xff8)
c: 29c00076 st.d $fp, $sp, 0
10: 00150076 move $fp, $sp
14: 02849c13 addi.w $t7, $zero, 295(0x127)
18: 58002dd3 beq $t2, $t7, 44(0x2c) # 0x44
1c: 03400000 andi $zero, $zero, 0x0
20: 64000c00 bge $zero, $zero, 12(0xc) # 0x2c
24: 03400000 andi $zero, $zero, 0x0
28: 03400000 andi $zero, $zero, 0x0
2c: 002a0006 break 0x6
30: 02ffe063 addi.d $sp, $sp, -8(0xff8)
34: 29c00061 st.d $ra, $sp, 0
38: 02ffe063 addi.d $sp, $sp, -8(0xff8)
3c: 29c00076 st.d $fp, $sp, 0
40: 00150076 move $fp, $sp
44: 00150084 move $a0, $a0
48: 00413084 slli.d $a0, $a0, 0xc
4c: 00119084 sub.d $a0, $a0, $a0
50: 28c00076 ld.d $fp, $sp, 0
54: 28c02061 ld.d $ra, $sp, 8(0x8)
58: 02c04063 addi.d $sp, $sp, 16(0x10)
5c: 4c000020 jirl $zero, $ra, 0
The main part is the following three instructions:
44: 00150084 move $a0, $a0
48: 00413084 slli.d $a0, $a0, 0xc
4c: 00119084 sub.d $a0, $a0, $a0
You can see here that the output register (from ToOutRegister64
) and temporary register for add64 and sub64 (from ToRegister64
) are both allocated to a0. For this case, the return value becomes a0 - a0
, which is 0
.
MIPS has the same problematic code. (See js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp:454
) Therefore, MIPS also gives the wrong compilation result.
Updated•2 years ago
|
Comment 1•2 years ago
|
||
Zhao Jiazhong, can you take a look at this?
Assignee | ||
Comment 2•2 years ago
|
||
Thanks for your information! I will have a look at it.
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
![]() |
||
Comment 4•2 years ago
|
||
[loong64][mips64] Fix register conflict in MulI64. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7bce553aae89a68d0e3e403a1ee365b0f88da427
https://hg.mozilla.org/mozilla-central/rev/7bce553aae89
Comment 5•2 years ago
|
||
Is this something we should consider for backport to the ESR115 branch? Not sure which Firefox releases are of interest for this architecture :-)
Assignee | ||
Comment 6•2 years ago
|
||
Comment on attachment 9340934 [details]
Bug 1836708 - [loong64][mips64] Fix register conflict in MulI64. r=jandem
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Some operation's result may not meet WebAssembly's spec on LoongArch64 and MIPS64 machines now。
- User impact if declined: Unpredictable errors in wasm program may occur to users.
- Fix Landed on Version: 116
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's a small change that only affects LoongArch64 and MIPS64 architecture.
Assignee | ||
Comment 7•2 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
Is this something we should consider for backport to the ESR115 branch? Not sure which Firefox releases are of interest for this architecture :-)
Yes, it would be great if this change could be backported to the ESR115 branch. And I have filed ESR uplift approval request, thanks!
Updated•2 years ago
|
Comment 8•2 years ago
|
||
Comment on attachment 9340934 [details]
Bug 1836708 - [loong64][mips64] Fix register conflict in MulI64. r=jandem
Approved for 115.1esr
Comment 9•2 years ago
|
||
uplift |
Updated•2 years ago
|
Updated•2 years ago
|
Updated•1 years ago
|
Comment 10•1 year ago
|
||
Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons
Description
•