Repository navigation
perf(native): keep attached machine code in file mappings - #2480
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
tignear
added
the
merge-ready
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11e6b51e1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| &self._mmap | ||
| // Safety: either owned mapping or external owner keeps this immutable | ||
| // executable region alive for the lifetime of this borrow. | ||
| unsafe { std::slice::from_raw_parts(self.fn_ptr as *const u8, self.code_len) } |
There was a problem hiding this comment.
Because fn_ptr is a public writable field, safe callers can swap it between two JitCode instances and then drop one instance. image() now constructs its slice from that replaceable pointer while retaining the original allocation's length and owner, so this sequence can produce a slice into unmapped memory; even without dropping either instance, differing allocation sizes can cause an out-of-bounds slice. Previously, image() borrowed the owned mapping directly and was unaffected by changes to fn_ptr. Retain a private immutable region-base pointer for both image() and entry_ptr() rather than deriving their addresses from the public entry pointer.
Useful? React with 👍 / 👎.
tignear
removed
the
merge-ready
|
Queued — the merge queue status continues in this comment ↓. |
|
This pull request is part of a Mergify stack:
|
Loading an attached image allocated an owned copy of its machine code before copying it again into executable memory. Retain immutable mapped code with Arc ownership and use aligned file pages directly on Linux x86-64. Unsupported or unaligned mappings retain the anonymous executable allocation path. Align newly attached code to 4 KiB, or 2 MiB for large images. A checksummed optional prefix descriptor records the original runtime length so replacement across Postcard length boundaries cannot accumulate padding. The Postcard payload, generic Serde representation and container version remain compatible. Keep image bytes and offset resolution tied to a private immutable base, even when callers replace the public entry pointer. Cover both owned and external backing lifetimes with entry-swap and owner-drop regressions. Validation: 20 native units, 627 x86 backend units, 36 native image/cache consumer tests, 68 VPI unit/API tests, 10 cocotb integrations, 330 native common cases (including wide, four-state and parallel execution), affected-target clippy with warnings denied, and formatting. Regressions cover retained owner lifetimes, legacy payload encoding, bounded replacement and corrupt padding. Change-Id: Idc33e7f4db1b90e2cf48d03f0fd39049aebda302
11e6b51 to
943ba42
Compare
Revision history
|
tignear
added
the
merge-ready
✅ Merged into
|
| Condition | Required by | |
|---|---|---|
| ✅ | check-success = Conventional Commit title |
— |
| ✅ | check-success = Queue validation |
— |
| ✅ | any of: check-success = @github-actions/Lint & Format, check-neutral = @github-actions/Lint & Format, check-skipped = @github-actions/Lint & Format |
GitHub repository ruleset rule master |
| ✅ | any of: check-success = @github-actions/Rust Test & NAPI Build, check-neutral = @github-actions/Rust Test & NAPI Build, check-skipped = @github-actions/Rust Test & NAPI Build |
GitHub repository ruleset rule master |
| ✅ | any of: check-success = @github-actions/JS Test (Ubuntu), check-neutral = @github-actions/JS Test (Ubuntu), check-skipped = @github-actions/JS Test (Ubuntu) |
GitHub repository ruleset rule master |
| ✅ | any of: check-success = @github-actions/Conventional Commit title, check-neutral = @github-actions/Conventional Commit title, check-skipped = @github-actions/Conventional Commit title |
GitHub repository ruleset rule master |
| ✅ | any of: check-success = @github-actions/Queue validation, check-neutral = @github-actions/Queue validation, check-skipped = @github-actions/Queue validation |
GitHub repository ruleset rule master |
Times in UTC · dashboard · how the merge queue works
Loading an attached image allocated an owned copy of its machine code before
copying it again into executable memory. Retain immutable mapped code with Arc
ownership and use aligned file pages directly on Linux x86-64. Unsupported or
unaligned mappings retain the anonymous executable allocation path.
Align newly attached code to 4 KiB, or 2 MiB for large images. A checksummed
optional prefix descriptor records the original runtime length so replacement
across Postcard length boundaries cannot accumulate padding. The Postcard
payload, generic Serde representation and container version remain compatible.
Keep image bytes and offset resolution tied to a private immutable base, even
when callers replace the public entry pointer. Cover both owned and external
backing lifetimes with entry-swap and owner-drop regressions.
Validation: 20 native units, 627 x86 backend units, 36 native image/cache
consumer tests, 68 VPI unit/API tests, 10 cocotb integrations, 330 native common
cases (including wide, four-state and parallel execution), affected-target
clippy with warnings denied, and formatting. Regressions cover retained owner
lifetimes, legacy payload encoding, bounded replacement and corrupt padding.