[SUB-BISECT] bcd47e7f3fe7 config-only (OptionsList.h + wasm.json + generator)#192
Closed
Jarred-Sumner wants to merge 9 commits into
Closed
[SUB-BISECT] bcd47e7f3fe7 config-only (OptionsList.h + wasm.json + generator)#192Jarred-Sumner wants to merge 9 commits into
Jarred-Sumner wants to merge 9 commits into
Conversation
This reverts commit f6f27b7.
…aths" This reverts commit c888ae4.
…f it does not call functions" This reverts commit ef40cab.
… size in each handler" This reverts commit 723bdac.
This reverts commit 32161b3.
…sed byte" This reverts commit 39def58.
…ry access" This reverts commit f197548.
This reverts commit bcd47e7. This is the 8th and final IPInt-touching commit from the d550dd3 upstream merge. With this revert stacked on the 7 prior reverts, InPlaceInterpreter64.asm, InPlaceInterpreter.asm, offlineasm/x86.rb, and WasmIPIntGenerator.cpp are now byte-identical to d924138 (the previous upstream sync point). If the Windows-x64 pglite OOB still reproduces on this preview, the IPInt source path is provably unchanged from the last-good build and the cause is elsewhere. Conflict resolution: _conversion_prefix guard restored to 0x12 with the decodeLEBVarUInt32 macro the earlier reverts already brought back.
…sHeader.py from bcd47e7 Config-only slice. This grows OptionsStorage by one bool (shifting JSC::Config fields after it) and adds Ext1OpType enum entries 0x13-0x16. Nothing else from bcd47e7. If this fails pglite on Windows -> the bug is the OptionsStorage size change shifting a JSC::Config field that something reads at a stale offset (or the generated WasmOps.h enum). If this passes -> the bug is in WasmBBQJIT*/WasmOMGIRGenerator/ WasmFunctionParser/assembler additions (compare against #191).
WalkthroughThis pull request removes wide integer arithmetic operations from WebAssembly, eliminates carry/borrow assembly instructions, refactors the In-Place Interpreter (IPInt) to use a pinned locals pointer and restructured LEB128 metadata encoding, and updates associated code generators and debugger integration. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h (1)
5066-5074:⚠️ Potential issue | 🟠 MajorThis commit violates its stated config-only scope by including MacroAssemblerX86_64.h.
The commit message declares "Config-only slice. This grows OptionsStorage by one bool... Nothing else from bcd47e7," listing only OptionsList.h, wasm.json, and generateWasmOpsHeader.py. However, the actual changes include Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h, which contains assembler logic (carry/borrow operations at lines 5071–5078).
Including assembler changes alongside config changes introduces a non-config variable into the bisect and invalidates attribution. Either revert the assembler file to match the baseline, or reconsider the commit's scope to reflect what was actually changed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h` around lines 5066 - 5074, The change illegally touched assembler logic in MacroAssemblerX86_64.h (notably the add64(Address, RegisterID)/add64(RegisterID, RegisterID) additions and the carry/borrow-related code near the carry/borrow operations), which violates the commit's "config-only" scope; either revert MacroAssemblerX86_64.h to the baseline (remove the assembler logic changes so only OptionsList.h/wasm.json/generateWasmOpsHeader.py remain) or split this work into a separate functional commit and update the commit message to reflect the assembler changes (ensure the file MacroAssemblerX86_64.h no longer contains the added assembler modifications like add64 overloads and any carry/borrow alterations unless you create a new, properly scoped commit).Source/JavaScriptCore/offlineasm/instructions.rb (1)
352-390:⚠️ Potential issue | 🟠 MajorThis change set breaks the “config-only” bisect isolation.
Lines 352–390 modify offlineasm opcode tables, which is outside the stated sub-bisect scope. That makes pass/fail results ambiguous (config shift vs opcode-table effects). Please remove this file from this bisect slice (or split it into a separate bisect step).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/offlineasm/instructions.rb` around lines 352 - 390, The change touches opcode tables X86_INSTRUCTIONS and X86_SIMD_INSTRUCTIONS which breaks a config-only bisect; revert the modifications to those symbols in this bisect slice (undo the added/removed entries in X86_INSTRUCTIONS and keep X86_SIMD_INSTRUCTIONS unchanged) and move the opcode-table edits into a separate commit/step, or remove this file from the current bisect so the slice remains config-only; ensure the separate commit clearly contains only the X86_INSTRUCTIONS/X86_SIMD_INSTRUCTIONS edits.Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.cpp (1)
478-492:⚠️ Potential issue | 🟡 MinorType inconsistency with other exception handlers.
The
rethrow_exceptionfunction declaresIPIntStackEntry* plbut uses the same access pattern (pl[callee->localSizeToAlloc() + tryDepth - 1]) as the other exception handlers that useIPIntLocal* pl. This matches the header declaration issue flagged earlier.If
IPIntStackEntryandIPIntLocalare indeed compatible types, consider usingIPIntLocal*here for consistency withretrieve_and_clear_exception,retrieve_clear_and_push_exception, andretrieve_clear_and_push_exception_and_arguments.Suggested fix for type consistency
-WASM_IPINT_EXTERN_CPP_DECL(rethrow_exception, CallFrame* callFrame, IPIntStackEntry* pl, unsigned tryDepth) +WASM_IPINT_EXTERN_CPP_DECL(rethrow_exception, CallFrame* callFrame, IPIntLocal* pl, unsigned tryDepth)And correspondingly in the header file (line 81):
-WASM_IPINT_EXTERN_CPP_HIDDEN_DECL(rethrow_exception, CallFrame*, IPIntStackEntry* pl, unsigned tryDepth); +WASM_IPINT_EXTERN_CPP_HIDDEN_DECL(rethrow_exception, CallFrame*, IPIntLocal* pl, unsigned tryDepth);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.cpp` around lines 478 - 492, The rethrow_exception function uses the same indexing pattern as other handlers but takes IPIntStackEntry* pl, causing a type inconsistency; change the parameter type to IPIntLocal* pl in rethrow_exception (and update the corresponding declaration in the header) to match retrieve_and_clear_exception, retrieve_clear_and_push_exception, and retrieve_clear_and_push_exception_and_arguments, while keeping the same access expression pl[callee->localSizeToAlloc() + tryDepth - 1] (ensure IPIntLocal and IPIntStackEntry are compatible before switching and adjust the header prototype accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/wasm/WasmBBQJIT.cpp`:
- Around line 1034-1037: The change altered BBQ temporary indexing logic in
BBQJIT::topValue (and the related change at the call-result materialization
around the other modified spot) which must not be changed on this config-only
bisect branch; restore the original temp-index computation by reverting the
modified expression that constructs Value::fromTemp (i.e., remove the new
addition/subtraction of
enclosedHeight()/implicitSlots()/expressionStack().size() logic) and reintroduce
the previous indexing expression used for temp/materialized call results so BBQ
temp indexing remains identical to the working commit (ensure you update both
BBQJIT::topValue and the other call-result site to the original computation).
In `@Source/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.cpp`:
- Around line 95-103: The IPInt::InstructionLengthMetadata written in
WasmFunctionIPIntMetadataGenerator.cpp's fast path incorrectly stores a bit from
the constant into .length; change the .length assignment in the short-i32.const
branch to store the actual instruction length (use safeCast<uint8_t>(length))
instead of safeCast<uint8_t>((value >> 7) & 1), so that the metadata (written
via m_metadata.grow and WRITE_TO_METADATA) consistently records the instruction
length for IPInt::InstructionLengthMetadata.
In `@Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp`:
- Around line 728-733: The vector-immediate producers (e.g.,
IPIntGenerator::addSIMDShuffle and the similar function at the 780-784 region)
currently only record instruction length and do not emit the new 128-bit
metadata or payload; update these functions to create and append
Const128Metadata and call addLEB128V128Constant() with the v128 immediate (the
128-bit shuffle mask or v128.const value) before/when calling
m_metadata->addLength/getCurrentInstructionLength(), so the Const128Metadata is
emitted and the payload written using addLEB128V128Constant(); reference the
symbols IPIntGenerator::addSIMDShuffle, Const128Metadata, and
addLEB128V128Constant when making the changes.
---
Outside diff comments:
In `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h`:
- Around line 5066-5074: The change illegally touched assembler logic in
MacroAssemblerX86_64.h (notably the add64(Address, RegisterID)/add64(RegisterID,
RegisterID) additions and the carry/borrow-related code near the carry/borrow
operations), which violates the commit's "config-only" scope; either revert
MacroAssemblerX86_64.h to the baseline (remove the assembler logic changes so
only OptionsList.h/wasm.json/generateWasmOpsHeader.py remain) or split this work
into a separate functional commit and update the commit message to reflect the
assembler changes (ensure the file MacroAssemblerX86_64.h no longer contains the
added assembler modifications like add64 overloads and any carry/borrow
alterations unless you create a new, properly scoped commit).
In `@Source/JavaScriptCore/offlineasm/instructions.rb`:
- Around line 352-390: The change touches opcode tables X86_INSTRUCTIONS and
X86_SIMD_INSTRUCTIONS which breaks a config-only bisect; revert the
modifications to those symbols in this bisect slice (undo the added/removed
entries in X86_INSTRUCTIONS and keep X86_SIMD_INSTRUCTIONS unchanged) and move
the opcode-table edits into a separate commit/step, or remove this file from the
current bisect so the slice remains config-only; ensure the separate commit
clearly contains only the X86_INSTRUCTIONS/X86_SIMD_INSTRUCTIONS edits.
In `@Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.cpp`:
- Around line 478-492: The rethrow_exception function uses the same indexing
pattern as other handlers but takes IPIntStackEntry* pl, causing a type
inconsistency; change the parameter type to IPIntLocal* pl in rethrow_exception
(and update the corresponding declaration in the header) to match
retrieve_and_clear_exception, retrieve_clear_and_push_exception, and
retrieve_clear_and_push_exception_and_arguments, while keeping the same access
expression pl[callee->localSizeToAlloc() + tryDepth - 1] (ensure IPIntLocal and
IPIntStackEntry are compatible before switching and adjust the header prototype
accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8a98750-93a6-4148-a904-939fabd6ba2f
📒 Files selected for processing (33)
JSTests/wasm/ipint-tests/ipint-test-leb-decode.jsJSTests/wasm/ipint-tests/ipint-test-simd-memory.jsJSTests/wasm/ipint-tests/ipint-test-simd-multi-byte-leb.jsJSTests/wasm/stress/atomic-cmpxchg-large-offset.jsJSTests/wasm/stress/memory64-atomics.jsJSTests/wasm/stress/wide-arithmetic.jsSource/JavaScriptCore/assembler/MacroAssemblerARM64.hSource/JavaScriptCore/assembler/MacroAssemblerX86_64.hSource/JavaScriptCore/assembler/X86Assembler.hSource/JavaScriptCore/llint/InPlaceInterpreter.asmSource/JavaScriptCore/llint/InPlaceInterpreter.cppSource/JavaScriptCore/llint/InPlaceInterpreter.hSource/JavaScriptCore/llint/InPlaceInterpreter64.asmSource/JavaScriptCore/offlineasm/arm64.rbSource/JavaScriptCore/offlineasm/instructions.rbSource/JavaScriptCore/offlineasm/x86.rbSource/JavaScriptCore/wasm/WasmBBQJIT.cppSource/JavaScriptCore/wasm/WasmBBQJIT.hSource/JavaScriptCore/wasm/WasmBBQJIT64.cppSource/JavaScriptCore/wasm/WasmConstExprGenerator.cppSource/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.cppSource/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.hSource/JavaScriptCore/wasm/WasmFunctionParser.hSource/JavaScriptCore/wasm/WasmIPIntGenerator.cppSource/JavaScriptCore/wasm/WasmIPIntGenerator.hSource/JavaScriptCore/wasm/WasmIPIntSlowPaths.cppSource/JavaScriptCore/wasm/WasmIPIntSlowPaths.hSource/JavaScriptCore/wasm/WasmOMGIRGenerator.cppSource/JavaScriptCore/wasm/debugger/WasmDebugServerUtilities.cppSource/JavaScriptCore/wasm/debugger/WasmDebugServerUtilities.hSource/JavaScriptCore/wasm/debugger/WasmExecutionHandler.cppSource/JavaScriptCore/wasm/debugger/WasmExecutionHandler.hSource/JavaScriptCore/wasm/debugger/WasmQueryHandler.cpp
💤 Files with no reviewable changes (14)
- JSTests/wasm/ipint-tests/ipint-test-simd-multi-byte-leb.js
- JSTests/wasm/ipint-tests/ipint-test-leb-decode.js
- JSTests/wasm/ipint-tests/ipint-test-simd-memory.js
- Source/JavaScriptCore/offlineasm/x86.rb
- JSTests/wasm/stress/memory64-atomics.js
- Source/JavaScriptCore/wasm/WasmConstExprGenerator.cpp
- Source/JavaScriptCore/offlineasm/arm64.rb
- JSTests/wasm/stress/wide-arithmetic.js
- Source/JavaScriptCore/assembler/X86Assembler.h
- JSTests/wasm/stress/atomic-cmpxchg-large-offset.js
- Source/JavaScriptCore/wasm/WasmOMGIRGenerator.cpp
- Source/JavaScriptCore/wasm/WasmFunctionParser.h
- Source/JavaScriptCore/assembler/MacroAssemblerARM64.h
- Source/JavaScriptCore/wasm/WasmBBQJIT64.cpp
Comment on lines
+1034
to
1037
| Value BBQJIT::topValue(TypeKind type) | ||
| { | ||
| return Value::fromTemp(type, currentControlData().enclosedHeight() + currentControlData().implicitSlots() + m_parser->expressionStack().size() + offset); | ||
| return Value::fromTemp(type, currentControlData().enclosedHeight() + currentControlData().implicitSlots() + m_parser->expressionStack().size()); | ||
| } |
There was a problem hiding this comment.
Don't change BBQ temp indexing on a config-only bisect branch.
Lines 1034-1037 and Line 4333 alter how BBQ materializes temp indices for values and call results. The PR objective explicitly says BBQ/OMG/IPInt generator code should stay at the working commit, so this behavior change invalidates the bisect's isolation even if the new indexing is internally consistent.
Also applies to: 4333-4333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/wasm/WasmBBQJIT.cpp` around lines 1034 - 1037, The
change altered BBQ temporary indexing logic in BBQJIT::topValue (and the related
change at the call-result materialization around the other modified spot) which
must not be changed on this config-only bisect branch; restore the original
temp-index computation by reverting the modified expression that constructs
Value::fromTemp (i.e., remove the new addition/subtraction of
enclosedHeight()/implicitSlots()/expressionStack().size() logic) and reintroduce
the previous indexing expression used for temp/materialized call results so BBQ
temp indexing remains identical to the working commit (ensure you update both
BBQJIT::topValue and the other call-result site to the original computation).
Comment on lines
+95
to
+103
| if (type.isI32()) { | ||
| size_t size = m_metadata.size(); | ||
| if (length == 2) { | ||
| IPInt::InstructionLengthMetadata mdConst { | ||
| .length = safeCast<uint8_t>((value >> 7) & 1) | ||
| }; | ||
| m_metadata.grow(size + sizeof(mdConst)); | ||
| WRITE_TO_METADATA(m_metadata.mutableSpan().data() + size, mdConst, IPInt::InstructionLengthMetadata); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'InstructionLengthMetadata|Const32Metadata|I32Const|i32\.const|\.length\b' Source/JavaScriptCore/wasmRepository: oven-sh/WebKit
Length of output: 50370
🏁 Script executed:
find Source/JavaScriptCore/wasm -name "*IPInt*" -type f | head -20Repository: oven-sh/WebKit
Length of output: 523
🏁 Script executed:
rg -n "InstructionLengthMetadata" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -A 5 -B 2 | head -100Repository: oven-sh/WebKit
Length of output: 417
🏁 Script executed:
rg -n "\.length\s*=" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp | grep -i "metadata\|instruction" | head -20Repository: oven-sh/WebKit
Length of output: 1475
🏁 Script executed:
rg -n "i32\.const|addLEB128ConstantAndLengthForType" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -B 3 -A 10 | head -150Repository: oven-sh/WebKit
Length of output: 1347
🏁 Script executed:
rg -n "InstructionLengthMetadata\|addLEB128ConstantAndLengthForType" Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.cpp -B 3 -A 8 | head -120Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
rg -n "metadata.*\.length|\.length.*metadata" Source/JavaScriptCore/wasm/ --type cpp | head -30Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
rg -n "struct.*Metadata|class.*Metadata" Source/JavaScriptCore/wasm/ -A 5 | grep -A 5 "InstructionLengthMetadata\|Const32Metadata"Repository: oven-sh/WebKit
Length of output: 8483
🏁 Script executed:
rg -n "op.i32Const\|case.*0x41\|OpType::I32Const" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -B 2 -A 15 | head -80Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
find Source/JavaScriptCore/wasm -name "*.h" -exec grep -l "InstructionLengthMetadata\|IPInt" {} \; | head -10Repository: oven-sh/WebKit
Length of output: 1130
🏁 Script executed:
rg -n "read.*Metadata\|advance.*metadata\|metadata.*read\|load.*Metadata" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -A 3 -B 1 | head -100Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
rg -n "metadataPtr|metadata\[|\.length" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp | grep -v "getCurrentInstructionLength\|addLength" | head -50Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
cat Source/JavaScriptCore/wasm/WasmIPIntGenerator.h | sed -n '142,157p'Repository: oven-sh/WebKit
Length of output: 490
🏁 Script executed:
rg -n "Const32Metadata|short.*i32\|small.*const" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -B 3 -A 8 | head -120Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
rg -n "\.instructionLength|InstructionLengthMetadata" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp | grep -v "\.instructionLength = " | head -40Repository: oven-sh/WebKit
Length of output: 159
🏁 Script executed:
rg -n "i32.*const|0x41" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -B 5 -A 10 | grep -A 15 "addConstant"Repository: oven-sh/WebKit
Length of output: 40
🏁 Script executed:
rg -n "addConstant" Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp -B 2 -A 8Repository: oven-sh/WebKit
Length of output: 974
Fix .length field for short-i32.const to store instruction length, not constant bits.
The fast path at lines 98–103 writes InstructionLengthMetadata with .length = safeCast<uint8_t>((value >> 7) & 1), storing a bit from the constant value instead of the actual instruction length. All other paths in the same function correctly store the actual length. The metadata structure is documented as carrying "length of current instruction" and the design comment indicates the value is embedded in bytecode while metadata holds only the instruction length. Using constant bits instead breaks this contract.
Proposed fix
if (length == 2) {
IPInt::InstructionLengthMetadata mdConst {
- .length = safeCast<uint8_t>((value >> 7) & 1)
+ .length = safeCast<uint8_t>(length)
};
m_metadata.grow(size + sizeof(mdConst));
WRITE_TO_METADATA(m_metadata.mutableSpan().data() + size, mdConst, IPInt::InstructionLengthMetadata);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (type.isI32()) { | |
| size_t size = m_metadata.size(); | |
| if (length == 2) { | |
| IPInt::InstructionLengthMetadata mdConst { | |
| .length = safeCast<uint8_t>((value >> 7) & 1) | |
| }; | |
| m_metadata.grow(size + sizeof(mdConst)); | |
| WRITE_TO_METADATA(m_metadata.mutableSpan().data() + size, mdConst, IPInt::InstructionLengthMetadata); | |
| } else { | |
| if (type.isI32()) { | |
| size_t size = m_metadata.size(); | |
| if (length == 2) { | |
| IPInt::InstructionLengthMetadata mdConst { | |
| .length = safeCast<uint8_t>(length) | |
| }; | |
| m_metadata.grow(size + sizeof(mdConst)); | |
| WRITE_TO_METADATA(m_metadata.mutableSpan().data() + size, mdConst, IPInt::InstructionLengthMetadata); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.cpp` around
lines 95 - 103, The IPInt::InstructionLengthMetadata written in
WasmFunctionIPIntMetadataGenerator.cpp's fast path incorrectly stores a bit from
the constant into .length; change the .length assignment in the short-i32.const
branch to store the actual instruction length (use safeCast<uint8_t>(length))
instead of safeCast<uint8_t>((value >> 7) & 1), so that the metadata (written
via m_metadata.grow and WRITE_TO_METADATA) consistently records the instruction
length for IPInt::InstructionLengthMetadata.
Comment on lines
728
to
733
| [[nodiscard]] PartialResult IPIntGenerator::addSIMDShuffle(v128_t, ExpressionType, ExpressionType, ExpressionType&) | ||
| { | ||
| changeStackSize(-1); | ||
| m_metadata->addLength(getCurrentInstructionLength()); | ||
| return { }; | ||
| } |
There was a problem hiding this comment.
Vector-immediate metadata is still not emitted.
Const128Metadata and addLEB128V128Constant() were added, but both vector-immediate producers here still write only instruction length. That leaves the new 128-bit metadata path unused and drops the payload for v128.const / shuffle-mask immediates.
Proposed fix
-[[nodiscard]] PartialResult IPIntGenerator::addSIMDShuffle(v128_t, ExpressionType, ExpressionType, ExpressionType&)
+[[nodiscard]] PartialResult IPIntGenerator::addSIMDShuffle(v128_t value, ExpressionType, ExpressionType, ExpressionType&)
{
changeStackSize(-1);
- m_metadata->addLength(getCurrentInstructionLength());
+ m_metadata->addLEB128V128Constant(value, getCurrentInstructionLength());
return { };
}
@@
-IPIntGenerator::ExpressionType IPIntGenerator::addSIMDConstant(v128_t)
+IPIntGenerator::ExpressionType IPIntGenerator::addSIMDConstant(v128_t value)
{
changeStackSize(1);
- m_metadata->addLength(getCurrentInstructionLength());
+ m_metadata->addLEB128V128Constant(value, getCurrentInstructionLength());
return { };
}Also applies to: 780-784
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp` around lines 728 - 733,
The vector-immediate producers (e.g., IPIntGenerator::addSIMDShuffle and the
similar function at the 780-784 region) currently only record instruction length
and do not emit the new 128-bit metadata or payload; update these functions to
create and append Const128Metadata and call addLEB128V128Constant() with the
v128 immediate (the 128-bit shuffle mask or v128.const value) before/when
calling m_metadata->addLength/getCurrentInstructionLength(), so the
Const128Metadata is emitted and the payload written using
addLEB128V128Constant(); reference the symbols IPIntGenerator::addSIMDShuffle,
Const128Metadata, and addLEB128V128Constant when making the changes.
Preview Builds
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Windows-x64-only WebAssembly OOB crash in pglite (
RuntimeError: Out of bounds memory accessatinvoke_vi) appeared after the WebKit upgrade in oven-sh/bun#29393.JSC_useWasmIPInt=0hides it. Bisect history:f8513c72(#190)1987e88f(#190)bcd47e7f3fe7(wide-arith) revertedbcd47e7f3fe7is the culprit. But disassembly ofLowLevelInterpreter.cpp.objfrom both Windows preview builds shows all 728ipint_*handlers are byte-identical after address normalization — only.balign 512padding differs. So the IPInt asm cannot be the cause; the bug is in one of the commit's other 13 files.This branch
Starts from working
1987e88f, re-applies only the config slice ofbcd47e7f3fe7:OptionsList.h— appendsuseWasmWideArithmeticbool. GrowsOptionsStorageby ≥1 byte, which shifts everyJSC::Configfield afteroptions:shellTimeoutCheckCallback,wasmDebuggerOnStop,wasmDebuggerOnResume,memoryDebuggerStopTheWorld,jsDebuggerStopTheWorld(BUN-specific),llint.exceptionInstructions[],llint.gateMap[].wasm.json— 4 wide-arith entries (extendedOp 19–22)generateWasmOpsHeader.py— addsFOR_EACH_WASM_WIDE_ARITHMETIC_OPtoExt1OpTypeenum +makeStringAsm, offlineasm, BBQ/OMG/IPInt generators, FunctionParser, and assembler all stay at the working state.
Decision matrix (vs #191 which re-applies all 13 non-asm files)
OptionsStoragegrowth shifts aJSC::Configfield something reads at a stale offset, OR the generatedExt1OpTypeenumWasmBBQJIT*/WasmOMGIRGenerator/WasmFunctionParser.h/assembler/*Not for merge — bisect preview only. See #190 for the full thread.