[SUB-BISECT] bcd47e7f3fe7 C++-only (asm stays reverted)#191
Closed
Jarred-Sumner wants to merge 9 commits into
Closed
[SUB-BISECT] bcd47e7f3fe7 C++-only (asm stays reverted)#191Jarred-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.
…reverted) Disassembly proves all 728 IPInt handlers are byte-identical between the broken (f8513c7) and working (1987e88) Windows x64 builds, so the InPlaceInterpreter64.asm + offlineasm changes cannot be the cause. This branch keeps the asm/offlineasm reverted and re-applies only: OptionsList.h, wasm.json, generateWasmOpsHeader.py, WasmFunctionParser.h, WasmIPIntGenerator.cpp, WasmBBQJIT*.{h,cpp}, WasmOMGIRGenerator.cpp, WasmConstExprGenerator.cpp, assembler/MacroAssembler{X86_64,ARM64}.h, assembler/X86Assembler.h. If this fails pglite on Windows, the bug is in one of these files. If it passes, the disassembly comparison missed something.
WalkthroughThis pull request removes deprecated WebAssembly test files, refactors the IPInt interpreter to use a dedicated 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Source/JavaScriptCore/wasm/debugger/WasmDebugServerUtilities.cpp (1)
342-347:⚠️ Potential issue | 🟠 MajorInitialize
localsin the prologue-stop constructor too.This overload is still the one used for function-entry/prologue stops, but it leaves
StopData::localsunset. Frame-0 local queries now rely on that pointer, so locals inspection at a prologue breakpoint will keep seeing null/stale data unless this path also computeslocalsFromFrame(callFrame, callee).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/wasm/debugger/WasmDebugServerUtilities.cpp` around lines 342 - 347, The prologue-stop constructor StopData::StopData(IPIntCallee* callee, JSWebAssemblyInstance* instance, CallFrame* callFrame) fails to initialize StopData::locals, so prologue/frame-0 local inspection can read null/stale data; update that constructor to compute and assign locals by calling localsFromFrame(callFrame, callee) (same way the other StopData overload does) so StopData::locals is properly populated for prologue/function-entry stops.Source/JavaScriptCore/llint/InPlaceInterpreter.asm (1)
112-122:⚠️ Potential issue | 🔴 Critical
PLis not actually dedicated on ARMv7.Line 112 makes
PLandsc3aliases of the same register (t7). Any ARMv7 path that usessc3as scratch will clobber the locals base and corrupt local/rethrow-slot accesses. This needs a distinct register allocation, or ARMv7 needs to stay on the old layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/llint/InPlaceInterpreter.asm` around lines 112 - 122, PL (locals base) is incorrectly aliased to sc3 via t7, so using sc3 will clobber the locals base on ARMv7; update the register allocation so PL and sc3 are distinct (e.g., stop aliasing PL to t7 or pick a different temp for sc3 such as t6 or another free GPR) and ensure the ARMv7 path either uses the old register layout or the new layout with unique bindings for PL and sc3 (adjust the const PL, sc3, and any related aliases like t7 accordingly so locals/rethrow-slot accesses are not corrupted).Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp (3)
309-310:⚠️ Potential issue | 🟠 MajorAdd instruction-length metadata to
memory.sizeandmemory.growoperations.
addGrowMemoryandaddCurrentMemoryrecord only the memory index but omit the instruction-length metadata, unlike all other memory operations (addMemoryFill,addMemoryCopy,addMemoryInit) and SIMD operations in this file, which consistently callm_metadata->addLength(getCurrentInstructionLength()). This metadata is necessary to correctly advance past variable-length immediates during bytecode interpretation.Fix
[[nodiscard]] PartialResult IPIntGenerator::addGrowMemory(ExpressionType, ExpressionType&, uint8_t memoryIndex) { m_metadata->addMemoryIndex(memoryIndex); + m_metadata->addLength(getCurrentInstructionLength()); return { }; } [[nodiscard]] PartialResult IPIntGenerator::addCurrentMemory(ExpressionType&, uint8_t memoryIndex) { changeStackSize(1); m_metadata->addMemoryIndex(memoryIndex); + m_metadata->addLength(getCurrentInstructionLength()); return { }; }Also applies to: lines 1120–1130
🤖 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 309 - 310, The addGrowMemory and addCurrentMemory implementations record only the memory index and omit instruction-length metadata; update both functions (addGrowMemory and addCurrentMemory in WasmIPIntGenerator.cpp) to call m_metadata->addLength(getCurrentInstructionLength()) at the same point other memory ops do so (after recording the memory index/immediate) so the interpreter advances past variable-length immediates correctly; apply the same change in the corresponding code path later in the file where these functions/logic are duplicated.
318-325:⚠️ Potential issue | 🟠 MajorUpdate atomic operations to support memory64 offsets consistently with scalar load/store.
The atomic functions (
atomicLoad,atomicStore,atomicBinaryRMW,atomicCompareExchange,atomicWait,atomicNotify) currently accept onlyuint32_toffsets and useaddLEB128ConstantInt32AndLength(), which prevents them from representing offsets above 4 GiB in shared memory64. The scalarload()andstore()operations already implement the correct pattern: acceptinguint64_toffsets with a conditional check forisMemory64()to use the appropriate encoding. Atomic operations must follow the same approach per the WebAssembly threads specification.Suggested fix
-[[nodiscard]] PartialResult atomicLoad(ExtAtomicOpType, Type, ExpressionType, ExpressionType&, uint32_t, uint8_t); -[[nodiscard]] PartialResult atomicStore(ExtAtomicOpType, Type, ExpressionType, ExpressionType, uint32_t, uint8_t); -[[nodiscard]] PartialResult atomicBinaryRMW(ExtAtomicOpType, Type, ExpressionType, ExpressionType, ExpressionType&, uint32_t, uint8_t); -[[nodiscard]] PartialResult atomicCompareExchange(ExtAtomicOpType, Type, ExpressionType, ExpressionType, ExpressionType, ExpressionType&, uint32_t, uint8_t); -[[nodiscard]] PartialResult atomicWait(ExtAtomicOpType, ExpressionType, ExpressionType, ExpressionType, ExpressionType&, uint32_t, uint8_t); -[[nodiscard]] PartialResult atomicNotify(ExtAtomicOpType, ExpressionType, ExpressionType, ExpressionType&, uint32_t, uint8_t); +[[nodiscard]] PartialResult atomicLoad(ExtAtomicOpType, Type, ExpressionType, ExpressionType&, uint64_t, uint8_t); +[[nodiscard]] PartialResult atomicStore(ExtAtomicOpType, Type, ExpressionType, ExpressionType, uint64_t, uint8_t); +[[nodiscard]] PartialResult atomicBinaryRMW(ExtAtomicOpType, Type, ExpressionType, ExpressionType, ExpressionType&, uint64_t, uint8_t); +[[nodiscard]] PartialResult atomicCompareExchange(ExtAtomicOpType, Type, ExpressionType, ExpressionType, ExpressionType, ExpressionType&, uint64_t, uint8_t); +[[nodiscard]] PartialResult atomicWait(ExtAtomicOpType, ExpressionType, ExpressionType, ExpressionType, ExpressionType&, uint64_t, uint8_t); +[[nodiscard]] PartialResult atomicNotify(ExtAtomicOpType, ExpressionType, ExpressionType, ExpressionType&, uint64_t, uint8_t); - m_metadata->addLEB128ConstantInt32AndLength(offset, getCurrentInstructionLength()); + if (m_info.memory(memoryIndex).isMemory64()) + m_metadata->addLEB128ConstantInt64AndLength(offset, getCurrentInstructionLength()); + else + m_metadata->addLEB128ConstantInt32AndLength(static_cast<uint32_t>(offset), getCurrentInstructionLength());Also applies to: 1166-1215
🤖 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 318 - 325, The atomic operation declarations and implementations (atomicLoad, atomicStore, atomicBinaryRMW, atomicCompareExchange, atomicWait, atomicNotify) must accept uint64_t offsets (not uint32_t) and mirror the scalar load/store encoding: when emitting offset immediates, check isMemory64() and call addLEB128ConstantInt64AndLength() for memory64 or keep addLEB128ConstantInt32AndLength() for memory32; update all call sites to pass uint64_t offsets and ensure the emitted op encoding uses the correct offset encoding branch; leave atomicFence signature as-is. Include the changed function signatures (e.g., atomicLoad(..., uint64_t, uint8_t)) and corresponding encoding changes inside those routines so offsets above 4 GiB are representable for shared memory64.
249-259:⚠️ Potential issue | 🟠 MajorApply memory64 offset handling to SIMD memarg helpers.
Vector loads/stores use a
memargimmediate with offset encoding. The scalar memory operations (load,store) already branch onisMemory64()to emit either int64 or int32 metadata for the offset. The SIMD memarg helpers—addSIMDLoad,addSIMDStore,addSIMDLoadLane,addSIMDStoreLane, and their delegates (addSIMDLoadSplat,addSIMDLoadExtend,addSIMDLoadPad)—still takeuint32_t offsetand unconditionally emit int32 metadata, causing offsets above 4 GiB to be truncated in memory64 modules.Change all SIMD offset parameters from
uint32_ttouint64_tand add the sameisMemory64()branching logic used in scalar operations.Suggested fix pattern
-[[nodiscard]] PartialResult addSIMDLoad(ExpressionType, uint32_t, ExpressionType&, uint8_t); +[[nodiscard]] PartialResult addSIMDLoad(ExpressionType, uint64_t, ExpressionType&, uint8_t); -[[nodiscard]] PartialResult IPIntGenerator::addSIMDLoad(ExpressionType, uint32_t offset, ExpressionType&, uint8_t memoryIndex) +[[nodiscard]] PartialResult IPIntGenerator::addSIMDLoad(ExpressionType, uint64_t offset, ExpressionType&, uint8_t memoryIndex) { changeStackSize(0); m_metadata->addMemoryIndex(memoryIndex); - m_metadata->addLEB128ConstantInt32AndLength(offset, getCurrentInstructionLength()); + if (m_info.memory(memoryIndex).isMemory64()) + m_metadata->addLEB128ConstantInt64AndLength(offset, getCurrentInstructionLength()); + else + m_metadata->addLEB128ConstantInt32AndLength(static_cast<uint32_t>(offset), getCurrentInstructionLength()); return { }; }Apply the same pattern to
addSIMDStore,addSIMDLoadLane, andaddSIMDStoreLane.🤖 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 249 - 259, The SIMD memarg helpers currently take uint32_t offsets and always emit 32-bit offset metadata; update all SIMD offset parameters from uint32_t to uint64_t for addSIMDLoad, addSIMDStore, addSIMDLoadLane, addSIMDStoreLane and their delegates addSIMDLoadSplat, addSIMDLoadExtend, addSIMDLoadPad, then add the same isMemory64() branching used by scalar loads/stores to emit either int64 or int32 offset metadata (use isMemory64() to choose emitting 64-bit offset metadata vs 32-bit) so offsets >4GiB are preserved in memory64 modules.
🤖 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/llint/InPlaceInterpreter.asm`:
- Around line 1205-1209: The saved frame slot order for [PL, callee/ws0] must be
normalized before invoking the debugger slow path; update the sequence around
push PC, MC / push PL, ws0 and the preparation of a2/sp so that the memory
layout matches the expectation in _ipint_extern_handle_debugger_trap_if_needed
(sp[0]==PL, sp[1]==ws0) on all architectures—e.g., detect or account for the
platform-specific multi-register push order and swap or reorder the saved PL and
callee/ws0 slots (the values used to compute a2/sp passed to operationCall of
_ipint_extern_handle_debugger_trap_if_needed) so the handler never reads swapped
pointers.
In `@Source/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.cpp`:
- Around line 95-103: The code in the i32 fast path (inside the type.isI32()
branch) writes the wrong value into IPInt::InstructionLengthMetadata.length by
using (value >> 7) & 1; change this to store the actual instruction length (i.e.
2) when length == 2: set IPInt::InstructionLengthMetadata.mdConst.length =
safeCast<uint8_t>(2) (instead of the immediate bit), then continue to grow
m_metadata and WRITE_TO_METADATA as before so metadata-driven decoding remains
in sync; reference the IPInt::InstructionLengthMetadata struct, m_metadata,
WRITE_TO_METADATA, and the length variable when locating the fix.
---
Outside diff comments:
In `@Source/JavaScriptCore/llint/InPlaceInterpreter.asm`:
- Around line 112-122: PL (locals base) is incorrectly aliased to sc3 via t7, so
using sc3 will clobber the locals base on ARMv7; update the register allocation
so PL and sc3 are distinct (e.g., stop aliasing PL to t7 or pick a different
temp for sc3 such as t6 or another free GPR) and ensure the ARMv7 path either
uses the old register layout or the new layout with unique bindings for PL and
sc3 (adjust the const PL, sc3, and any related aliases like t7 accordingly so
locals/rethrow-slot accesses are not corrupted).
In `@Source/JavaScriptCore/wasm/debugger/WasmDebugServerUtilities.cpp`:
- Around line 342-347: The prologue-stop constructor
StopData::StopData(IPIntCallee* callee, JSWebAssemblyInstance* instance,
CallFrame* callFrame) fails to initialize StopData::locals, so prologue/frame-0
local inspection can read null/stale data; update that constructor to compute
and assign locals by calling localsFromFrame(callFrame, callee) (same way the
other StopData overload does) so StopData::locals is properly populated for
prologue/function-entry stops.
In `@Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp`:
- Around line 309-310: The addGrowMemory and addCurrentMemory implementations
record only the memory index and omit instruction-length metadata; update both
functions (addGrowMemory and addCurrentMemory in WasmIPIntGenerator.cpp) to call
m_metadata->addLength(getCurrentInstructionLength()) at the same point other
memory ops do so (after recording the memory index/immediate) so the interpreter
advances past variable-length immediates correctly; apply the same change in the
corresponding code path later in the file where these functions/logic are
duplicated.
- Around line 318-325: The atomic operation declarations and implementations
(atomicLoad, atomicStore, atomicBinaryRMW, atomicCompareExchange, atomicWait,
atomicNotify) must accept uint64_t offsets (not uint32_t) and mirror the scalar
load/store encoding: when emitting offset immediates, check isMemory64() and
call addLEB128ConstantInt64AndLength() for memory64 or keep
addLEB128ConstantInt32AndLength() for memory32; update all call sites to pass
uint64_t offsets and ensure the emitted op encoding uses the correct offset
encoding branch; leave atomicFence signature as-is. Include the changed function
signatures (e.g., atomicLoad(..., uint64_t, uint8_t)) and corresponding encoding
changes inside those routines so offsets above 4 GiB are representable for
shared memory64.
- Around line 249-259: The SIMD memarg helpers currently take uint32_t offsets
and always emit 32-bit offset metadata; update all SIMD offset parameters from
uint32_t to uint64_t for addSIMDLoad, addSIMDStore, addSIMDLoadLane,
addSIMDStoreLane and their delegates addSIMDLoadSplat, addSIMDLoadExtend,
addSIMDLoadPad, then add the same isMemory64() branching used by scalar
loads/stores to emit either int64 or int32 offset metadata (use isMemory64() to
choose emitting 64-bit offset metadata vs 32-bit) so offsets >4GiB are preserved
in memory64 modules.
🪄 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: 9286e060-c15c-4683-85b6-eeb3d97635a3
📒 Files selected for processing (24)
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/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/WasmFunctionIPIntMetadataGenerator.cppSource/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.hSource/JavaScriptCore/wasm/WasmIPIntGenerator.cppSource/JavaScriptCore/wasm/WasmIPIntGenerator.hSource/JavaScriptCore/wasm/WasmIPIntSlowPaths.cppSource/JavaScriptCore/wasm/WasmIPIntSlowPaths.hSource/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 (8)
- JSTests/wasm/stress/memory64-atomics.js
- JSTests/wasm/ipint-tests/ipint-test-leb-decode.js
- JSTests/wasm/stress/atomic-cmpxchg-large-offset.js
- JSTests/wasm/ipint-tests/ipint-test-simd-memory.js
- JSTests/wasm/ipint-tests/ipint-test-simd-multi-byte-leb.js
- Source/JavaScriptCore/offlineasm/arm64.rb
- Source/JavaScriptCore/offlineasm/x86.rb
- JSTests/wasm/stress/wide-arithmetic.js
Comment on lines
1205
to
1209
| push PC, MC | ||
| push ws0, ws0 # sp[0]=ws0 (unused), sp[1]=ws0 (IPIntCallee*), sp[2]=PC, sp[3]=MC | ||
| push PL, ws0 # sp[0]=PL, sp[1]=ws0 (IPIntCallee*), sp[2]=PC, sp[3]=MC | ||
| move cfr, a1 | ||
| move sp, a2 # a2 = pointer to saved [ws0, ws0, PC, MC] | ||
| move sp, a2 # a2 = pointer to saved [PL, ws0, PC, MC] | ||
| operationCall(macro() cCall3(_ipint_extern_handle_debugger_trap_if_needed) end) |
There was a problem hiding this comment.
Normalize the saved [PL, callee] layout before calling the debugger slow path.
This code assumes sp[0] == PL and sp[1] == ws0, but elsewhere in this file you already note that multi-register push order differs between ARM64 and X86_64. On X86_64, handle_debugger_trap_if_needed can read swapped pointers and treat the callee as the locals base.
💡 Suggested fix
macro handleDebuggerTrapIfNeeded()
push PC, MC
- push PL, ws0 # sp[0]=PL, sp[1]=ws0 (IPIntCallee*), sp[2]=PC, sp[3]=MC
+ if ARM64 or ARM64E
+ push PL, ws0 # sp[0]=PL, sp[1]=ws0, sp[2]=PC, sp[3]=MC
+ elsif X86_64
+ push ws0
+ push PL # force the same logical layout seen by C++
+ end
move cfr, a1
move sp, a2 # a2 = pointer to saved [PL, ws0, PC, MC]
operationCall(macro() cCall3(_ipint_extern_handle_debugger_trap_if_needed) end)
addp 4 * MachineRegisterSize, sp
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/llint/InPlaceInterpreter.asm` around lines 1205 - 1209,
The saved frame slot order for [PL, callee/ws0] must be normalized before
invoking the debugger slow path; update the sequence around push PC, MC / push
PL, ws0 and the preparation of a2/sp so that the memory layout matches the
expectation in _ipint_extern_handle_debugger_trap_if_needed (sp[0]==PL,
sp[1]==ws0) on all architectures—e.g., detect or account for the
platform-specific multi-register push order and swap or reorder the saved PL and
callee/ws0 slots (the values used to compute a2/sp passed to operationCall of
_ipint_extern_handle_debugger_trap_if_needed) so the handler never reads swapped
pointers.
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.
The 2-byte i32 fast path stores the wrong metadata value.
Line 99 writes (value >> 7) & 1 into InstructionLengthMetadata.length. That is a bit from the immediate, not the instruction length, so 2-byte immediates end up with length == 0/1 instead of 2, and metadata-driven decoding will drift immediately after the first such instruction.
💡 Suggested 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 {🤖 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 code in the i32 fast path (inside the type.isI32() branch)
writes the wrong value into IPInt::InstructionLengthMetadata.length by using
(value >> 7) & 1; change this to store the actual instruction length (i.e. 2)
when length == 2: set IPInt::InstructionLengthMetadata.mdConst.length =
safeCast<uint8_t>(2) (instead of the immediate bit), then continue to grow
m_metadata and WRITE_TO_METADATA as before so metadata-driven decoding remains
in sync; reference the IPInt::InstructionLengthMetadata struct, m_metadata,
WRITE_TO_METADATA, and the length variable when locating the fix.
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 — the only diff is 256 bytes of.balign 512padding before the atomic section. Conversion handler addresses0x35000–0x36100(slots0xFC 0x00–0x11) are identical in both. So the IPInt asm cannot be the cause.This branch
Starts from working
1987e88f(all 8 reverted), keepsInPlaceInterpreter64.asm+offlineasm/reverted, and re-applies only the C++/generator/assembler parts ofbcd47e7f3fe7:OptionsList.h(appendsuseWasmWideArithmeticbool → growsOptionsStorage)wasm.json+generateWasmOpsHeader.py(Ext1OpTypeenum entries 0x13–0x16)WasmFunctionParser.h(gated parser cases)WasmIPIntGenerator.cpp/WasmConstExprGenerator.cpp(stubs)WasmBBQJIT.{h,cpp}/WasmBBQJIT64.cpp(topValuerefactor + BBQ impls)WasmOMGIRGenerator.cpp(OMG impls)assembler/X86Assembler.h/MacroAssemblerX86_64.h/MacroAssemblerARM64.h(adcq/sbbq/addCarry64/subBorrow64)Decision matrix (vs #192 which re-applies only the config slice)
OptionsStoragegrowth orExt1OpTypeenumWasmBBQJIT*/WasmOMGIRGenerator/WasmFunctionParser.h/assembler/*Not for merge — bisect preview only. See #190 for the full thread.