Conversation
…tion Refactor `LoadImmJumpInd` instruction decoding to align with specification A.5.12. This involves replacing `decodeVarintPair` with `decodeImmediate2` for extracting immediate values (value and offset). Enhance the `djump` helper to accurately determine the number of jump table entries by dividing the jump table's byte length by `jumpTableEntrySize`. A guard has been added to prevent panics when `jumpTableEntrySize` is zero. Update `JITControlFlowTests` to reflect the new `LoadImmJumpInd` encoding. Re-enable the `v072_sandbox` fuzz test.
- Remove LEB128 varint encoding/decoding from instruction handlers - Add compact immediate encoding with little-endian bytes and length prefixes - Update BranchImm, StoreImm, LoadImm, Ecalli, and StoreImmInd instructions - Add decode_immediate_signed, clamp_imm_len, second_immediate_len helpers - Refactor instruction size calculation to use skip tables for variable-length opcodes - Update test helpers and Swift code to use new encoding format - Fix jump table encoding to use dense fixed-width entries per spec
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look solid and correctly implement the refactoring of JIT instruction decoding to use compact variable-length immediates with skip tables, removing the dependency on varint decoding. The macOS sandbox support using posix_spawn is also a good improvement. I found one minor issue with a hardcoded sleep in the child process manager.
…esolution - Replace Swift PVMOpcodes enum with CppHelper.Instructions alias for single source of truth - Add SandboxExecutableResolver to automatically find boka-sandbox executable across PATH and build directories - Implement fallback to in-process execution when sandbox not found (non-explicit path) - Reduce logging verbosity: change debug/error logs to trace to avoid spam - Update all JIT tests to use CppHelperInstructions - Remove PVMStressTests.swift - Add instruction size definitions for 2-register opcodes in C++ helper
- Set FD_CLOEXEC on socketpair FDs to prevent descriptor leakage into child processes - Treat unexpected EOF as normal shutdown path in IPC server (trace instead of error) - Convert page fault exit reason to panic trap in invokePVM - Add environment-controlled debug logging for sandbox (BOKA_SANDBOX_DEBUG) - Remove unconditional debug print statements from executor and IPC client
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #385 +/- ##
==========================================
+ Coverage 64.43% 73.57% +9.13%
==========================================
Files 438 441 +3
Lines 36678 37090 +412
==========================================
+ Hits 23634 27289 +3655
+ Misses 13044 9801 -3243 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit refactors the sandboxed executor to improve its robustness and fallback mechanism. - The `ExecutorFrontendSandboxed` now proactively checks for the sandbox executable's presence and falls back to in-process execution if it's missing or if `IPCError.brokenPipe` occurs during execution. - Extracted the in-process execution logic into a dedicated private method `executeInProcess` for better code organization. - The `SandboxExecutableResolver` introduces a new utility function `isExecutableAvailable` to reliably check for executable files, including searching in the system's PATH. - Removed hardcoded build subpaths from `SandboxExecutableResolver`, simplifying its default path resolution. - Updated the CI workflow to explicitly build the `boka-sandbox` executable and set its path in the `BOKA_SANDBOX_PATH` environment variable, ensuring its availability for sandboxed tests.
… CI build SandboxExecutableResolver now checks if an explicitly provided sandbox executable path is actually available, ensuring the `isExplicit` flag is accurate. Optimized CI workflow by removing a redundant `swift build` command when resolving the `boka-sandbox` path.
Improve how the `boka-sandbox` executable is located, particularly in test environments. - Prioritize `BOKA_SANDBOX_PATH` environment variable for explicit path configuration in tests. - Fallback to checking for a `boka-sandbox` sibling executable relative to the test binary. - Remove redundant `isExecutableAvailable` check in `SandboxExecutableResolver` for explicit paths. - Update test error messages to guide users on setting `BOKA_SANDBOX_PATH`.
Previously, IPC client read and write operations could block indefinitely, leading to unresponsive behavior. This change introduces a timeout mechanism for all IPC read and write operations within the `IPCClient`. It leverages `poll` to wait for file descriptor events (readability or writability) with a configurable deadline, preventing indefinite blocking. The CI workflow has been updated to explicitly build the `boka-sandbox` product and verify its existence, ensuring the sandbox executable is available for testing. A new test suite `IPCClientTests` has been added, including a test case to confirm that `sendExecuteRequest` correctly times out when no response is received from the peer.
Rust static libraries can export duplicate runtime symbols (e.g., rust_eh_personality) across archives. GNU ld rejects these by default, leading to linker errors. This change adds the -Wl,--allow-multiple-definition flag for Linux platforms to resolve this issue.
The Makefile's 'test' target has been refactored to simplify test output parsing. It now relies directly on the 'swift test' command's exit code for package-level pass/fail status, removing complex output string matching. CI workflow timeouts for benchmark and test jobs have been reduced from 60/180 minutes to 30 minutes to provide faster feedback and prevent excessively long runs. The '--quiet' flag was removed from benchmark baseline updates to improve visibility during benchmark execution.
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
1 participant
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.
No description provided.