cache llbc - #175
Merged
Merged
cache llbc#175
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request comprehensively migrates Charon installation and LLBC extraction from shell scripts to Python implementations with deterministic fingerprinting, and refactors the CI workflow to centralize artifact preparation in a shared job that both test jobs depend on and restore cached artifacts from. All documentation, error messages, and test guidance are updated to reference the new Python scripts. ChangesPython Script Migration and CI Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
marked this pull request as ready for review
June 13, 2026 17:25
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 (1)
.github/workflows/pyre-ci.yml (1)
68-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseon all checkout actions. The GitHub token persists by default in all three jobs (prepare-charon-llbc,cargo-test,pyre-check), creating an unnecessary security risk since no subsequent steps require repository write access.
.github/workflows/pyre-ci.yml#L68-L68: addwith: { persist-credentials: false }to theprepare-charon-llbccheckout step..github/workflows/pyre-ci.yml#L128-L128: addwith: { persist-credentials: false }to thecargo-testcheckout step..github/workflows/pyre-ci.yml#L170-L170: addwith: { persist-credentials: false }to thepyre-checkcheckout step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pyre-ci.yml at line 68, For each checkout step in .github/workflows/pyre-ci.yml add the GitHub Actions input to disable token persistence: at .github/workflows/pyre-ci.yml:68 (the prepare-charon-llbc checkout step using actions/checkout@de0fac2...), add a with block containing persist-credentials: false; at .github/workflows/pyre-ci.yml:128 (the cargo-test checkout step) add the same with: { persist-credentials: false }; and at .github/workflows/pyre-ci.yml:170 (the pyre-check checkout step) add with: { persist-credentials: false } so none of the three checkout actions leave the repo write token available to subsequent steps.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pyre-ci.yml:
- Around line 133-144: Add explicit cache-hit checks so a missing upstream cache
produces a clear error: in .github/workflows/pyre-ci.yml (lines 133-144) give
the two restore steps ids restore-charon and restore-llbc, then add a follow-up
conditional step that runs only when either
steps.restore-charon.outputs.cache-hit != 'true' or
steps.restore-llbc.outputs.cache-hit != 'true' and fails with an explicit "cache
miss from prepare-charon-llbc" message; repeat the same change in
.github/workflows/pyre-ci.yml (lines 185-196) (add the same ids there if absent
and a likewise conditional failing step) so both cargo-test and pyre-check jobs
detect and report upstream cache misses.
- Line 153: Replace the direct matrix template injection in the run command by
assigning the matrix value to an environment variable and using that variable in
the shell; specifically, set an env like BACKEND: ${{ matrix.backend }} and
change the run invocation from the literal "cargo test --all
--no-default-features --features ${{ matrix.backend }}" to use the environment
variable, e.g. "cargo test --all --no-default-features --features \"$BACKEND\""
so the matrix value is passed via a controlled env var rather than directly
injected into the shell.
---
Outside diff comments:
In @.github/workflows/pyre-ci.yml:
- Line 68: For each checkout step in .github/workflows/pyre-ci.yml add the
GitHub Actions input to disable token persistence: at
.github/workflows/pyre-ci.yml:68 (the prepare-charon-llbc checkout step using
actions/checkout@de0fac2...), add a with block containing persist-credentials:
false; at .github/workflows/pyre-ci.yml:128 (the cargo-test checkout step) add
the same with: { persist-credentials: false }; and at
.github/workflows/pyre-ci.yml:170 (the pyre-check checkout step) add with: {
persist-credentials: false } so none of the three checkout actions leave the
repo write token available to subsequent steps.
🪄 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: 9be39ce1-6b5e-4b68-91a2-44d7251daf27
📒 Files selected for processing (1)
.github/workflows/pyre-ci.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bad22e83f6
ℹ️ 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".
| cargo-test: | ||
| name: cargo test (${{ matrix.os }}, ${{ matrix.backend }}) | ||
| runs-on: ${{ matrix.os }} | ||
| needs: prepare-charon-llbc |
There was a problem hiding this comment.
If any prepare-charon-llbc matrix leg fails (for example the Windows Charon source build or cache restore), this needs makes every cargo-test matrix child skip; the same new dependency is also on pyre-check. GitHub documents that dependent jobs skipped after a failed need “do not report a failure” and a PR requiring that check “may not be blocked” (docs), so branch protection that already requires the existing cargo-test/pyre-check checks can merge without running tests unless the new prepare checks/aggregate are required or the consumers run with always() and fail explicitly.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
main
branch
2 times, most recently
from
June 14, 2026 00:20
f59d49f to
a679dc1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/pyre-ci.yml (2)
61-61: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
CHARON_VERSIONis duplicated in three job env blocks despite the single-source comment.Line 61, Line 125, and Line 189 repeat the same pin. A future one-site edit will force guaranteed downstream cache misses (and hard-fail due to the new cache-hit gates). Promote it to a top-level workflow
envand reference it from all jobs.♻️ Proposed cleanup
permissions: contents: read +env: + CHARON_VERSION: nightly-2026.05.29 jobs: prepare-charon-llbc: env: PYRE_SHARED_BUILD: ${{ github.workspace }}/.pyre-build - CHARON_VERSION: nightly-2026.05.29 cargo-test: env: PYRE_SHARED_BUILD: ${{ github.workspace }}/.pyre-build - CHARON_VERSION: nightly-2026.05.29 pyre-check: env: PYRE_SHARED_BUILD: ${{ github.workspace }}/.pyre-build - CHARON_VERSION: nightly-2026.05.29Also applies to: 125-125, 189-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pyre-ci.yml at line 61, The CHARON_VERSION environment variable is duplicated across three job env blocks (.github/workflows/pyre-ci.yml at lines 61, 125, and 189) with the identical value nightly-2026.05.29. To follow the single-source principle and ensure consistency, remove the CHARON_VERSION definition from all three job-level env blocks and add it once at the top-level workflow env section (before any jobs are defined). Then reference the workflow-level CHARON_VERSION variable from each job, which will inherit the value automatically.
64-67:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLLBC cache key currently ignores backend, so
dynasmlegs can consumecranelift-generated artifacts.At Line 64-Line 67, the producer matrix no longer has a backend dimension, and at Line 114 extraction runs without setting
CARGO_FEATURES(the extractor defaults backend selection unless overridden). But at Line 129-Line 130,cargo-teststill validates bothdynasmandcraneliftwhile restoring the same LLBC key at Line 159 (and similarly at Line 232 inpyre-check). This can silently validate the wrong backend artifact set.💡 Suggested direction
# prepare-charon-llbc strategy: matrix: os: [ubuntu-24.04, macos-latest, windows-latest] + backend: [dynasm, cranelift] - key: llbc-${{ runner.os }}-${{ runner.arch }}-${{ env.CHARON_VERSION }}-${{ hashFiles(...) }} + key: llbc-${{ runner.os }}-${{ runner.arch }}-${{ matrix.backend }}-${{ env.CHARON_VERSION }}-${{ hashFiles(...) }} - run: scripts/extract-llbc.sh pyre-object pyre-interpreter pyre-jit + env: + CARGO_FEATURES: ${{ matrix.backend }} + run: scripts/extract-llbc.sh pyre-object pyre-interpreter pyre-jit # cargo-test restore key should include backend too - key: llbc-${{ runner.os }}-${{ runner.arch }}-${{ env.CHARON_VERSION }}-${{ hashFiles(...) }} + key: llbc-${{ runner.os }}-${{ runner.arch }}-${{ matrix.backend }}-${{ env.CHARON_VERSION }}-${{ hashFiles(...) }}Also applies to: 109-114, 129-130, 159-159, 232-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pyre-ci.yml around lines 64 - 67, The LLBC cache key does not include the backend (dynasm vs cranelift) in its hash, allowing different backend test legs to incorrectly reuse artifacts built for the wrong backend. In the matrix definition at lines 64-67, add a backend dimension with values for both dynasm and cranelift. At lines 109-114 where extraction runs, ensure CARGO_FEATURES is set based on the backend matrix variable being tested. At lines 129-130 where cargo-test runs, configure it to use only the backend specified by the matrix variable. At lines 159 and 232 where the LLBC cache key is generated, include the backend matrix variable as part of the cache key hash to ensure dynasm and cranelift artifacts are cached separately. This ensures each backend's test pipeline uses the correct pre-built artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/pyre-ci.yml:
- Line 61: The CHARON_VERSION environment variable is duplicated across three
job env blocks (.github/workflows/pyre-ci.yml at lines 61, 125, and 189) with
the identical value nightly-2026.05.29. To follow the single-source principle
and ensure consistency, remove the CHARON_VERSION definition from all three
job-level env blocks and add it once at the top-level workflow env section
(before any jobs are defined). Then reference the workflow-level CHARON_VERSION
variable from each job, which will inherit the value automatically.
- Around line 64-67: The LLBC cache key does not include the backend (dynasm vs
cranelift) in its hash, allowing different backend test legs to incorrectly
reuse artifacts built for the wrong backend. In the matrix definition at lines
64-67, add a backend dimension with values for both dynasm and cranelift. At
lines 109-114 where extraction runs, ensure CARGO_FEATURES is set based on the
backend matrix variable being tested. At lines 129-130 where cargo-test runs,
configure it to use only the backend specified by the matrix variable. At lines
159 and 232 where the LLBC cache key is generated, include the backend matrix
variable as part of the cache key hash to ensure dynasm and cranelift artifacts
are cached separately. This ensures each backend's test pipeline uses the
correct pre-built artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 827fcfce-be4e-4938-a12b-97c353594e6d
📒 Files selected for processing (1)
.github/workflows/pyre-ci.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfbc1b359f
ℹ️ 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".
Comment on lines
+149
to
+150
| if not ({"lib", "bin", "custom-build"} & kinds): | ||
| continue |
There was a problem hiding this comment.
For local proc-macro dependencies in the closure, Cargo reports their target kind as proc-macro (e.g. pyre-object depends on majit-macros and pyre-macros), so this filter hashes only their Cargo.toml and skips their src/ trees. If a PR changes a macro that affects generated interpreter/object code, the CI LLBC cache key remains unchanged and prepare-charon-llbc can restore stale .ullbc instead of re-extracting, leaving tests to run against outdated Charon output. Include proc-macro targets in the hashed source set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pyre-ci.yml:
- Line 113: The step at line 113 that runs scripts/extract-llbc.py pyre-object
pyre-interpreter pyre-jit is missing the shell specification, which causes it to
default to PowerShell on windows-latest runners instead of bash like the other
script invocations in the workflow. Add shell: bash to this step to ensure
consistent behavior across all platforms and match the pattern used in the other
script invocations at lines 91, 95, 147, and 224.
In `@majit/charon-corpus/README.md`:
- Around line 24-33: The install snippet in the README shows
`scripts/install-charon.py` as a bare relative path, but since the README is
nested under `majit/charon-corpus/`, this path is incorrect and does not match
the directory structure. Update the path to `../../scripts/install-charon.py` to
use the same prefix that is already correctly used in the reproduction block
elsewhere in the README, ensuring the script reference works from the nested
directory location.
In `@pyre/check.py`:
- Around line 385-387: The bare scripts/extract-llbc.py command is too generic
and doesn't specify which LLBC targets to extract, making it unclear what users
need to regenerate. At pyre/check.py (lines 385-387), replace
scripts/extract-llbc.py with scripts/extract-llbc.py pyre-object
pyre-interpreter. At majit/majit-charon-reader/README.md (lines 57-61), replace
scripts/extract-llbc.py with scripts/extract-llbc.py pyre-object
pyre-interpreter. At
majit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rs
(lines 40-43), replace scripts/extract-llbc.py with scripts/extract-llbc.py
pyre-object pyre-interpreter pyre-jit. At
majit/majit-translate/tests/test_mir_dispatch.rs (lines 20-22), replace
scripts/extract-llbc.py with scripts/extract-llbc.py pyre-interpreter.
In `@scripts/extract-llbc.py`:
- Line 300: The for loop at the point where crates are iterated uses ALL_CRATES
as the default fallback when no crates argument is provided, but the main
function declares DEFAULT_CRATES as the default behavior for other command-line
options. Replace ALL_CRATES with DEFAULT_CRATES in the conditional expression to
ensure consistent default crate set behavior across all code paths, so that
running the script with no arguments extracts the same set of crates as the
declared defaults for --fingerprint and --list-inputs.
In `@scripts/install-charon.py`:
- Around line 95-96: The urllib.request.urlopen call in the download operation
does not specify a timeout parameter, which can cause the process to hang
indefinitely during network stalls. Add a timeout parameter to the urlopen()
call to prevent CI jobs from being blocked. Specify an appropriate timeout value
(in seconds) that allows sufficient time for the release download while
protecting against indefinite hangs from network issues.
- Around line 170-173: The tar.extractall() call on line 171 extracts archive
members without validating their paths, allowing potential path traversal
attacks where a crafted tarball could write files outside tmp_dir. Before
calling tar.extractall(), iterate through all members returned by
tar.getmembers() and validate that each member's name does not contain path
traversal sequences like ".." and that the normalized path remains within
tmp_dir. Filter out or reject any members with unsafe paths before proceeding
with the extraction.
🪄 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: 516bbdb1-3381-43f2-9ae4-c1043b910b90
📒 Files selected for processing (15)
.github/workflows/pyre-ci.ymlmajit/charon-corpus/README.mdmajit/majit-charon-reader/README.mdmajit/majit-charon-reader/src/lib.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rsmajit/majit-translate/tests/test_mir_dispatch.rsmajit/majit-translate/tests/test_mir_stress.rspyre/check.pypyre/pyre-jit-trace/build.rsscripts/charon-msvc-env.shscripts/extract-llbc.pyscripts/extract-llbc.shscripts/install-charon.pyscripts/install-charon.sh
💤 Files with no reviewable changes (3)
- scripts/charon-msvc-env.sh
- scripts/install-charon.sh
- scripts/extract-llbc.sh
Comment on lines
24
to
+33
| ```sh | ||
| scripts/install-charon.sh | ||
| scripts/install-charon.py | ||
| ../.pyre-build/charon/<platform>/charon toolchain-path # one-time nightly install (~1 min) | ||
| ``` | ||
|
|
||
| The script pins `CHARON_VERSION_DEFAULT="nightly-2026.05.29"` and installs | ||
| to a shared cache at `../.pyre-build/charon/<platform>` by default, so sibling | ||
| worktrees reuse the same Charon binary. Override with | ||
| `PYRE_SHARED_BUILD=/path/to/cache` or `CHARON_DEST=/path/to/bin`; use | ||
| `CHARON_VERSION=nightly-YYYY.MM.DD scripts/install-charon.sh` to bump. | ||
| `CHARON_VERSION=nightly-YYYY.MM.DD scripts/install-charon.py` to bump. |
There was a problem hiding this comment.
Fix the script path in the install snippet.
This README is nested under majit/charon-corpus/, so the bare scripts/install-charon.py path does not match the directory implied by the rest of the instructions. Use the same ../../scripts/install-charon.py prefix already used in the reproduction block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/charon-corpus/README.md` around lines 24 - 33, The install snippet in
the README shows `scripts/install-charon.py` as a bare relative path, but since
the README is nested under `majit/charon-corpus/`, this path is incorrect and
does not match the directory structure. Update the path to
`../../scripts/install-charon.py` to use the same prefix that is already
correctly used in the reproduction block elsewhere in the README, ensuring the
script reference works from the nested directory location.
Comment on lines
385
to
+387
| print(red("LLBC artefacts are missing under build/llbc/.")) | ||
| print("Run the extractor first, then re-run this script:") | ||
| print(" scripts/extract-llbc.sh") | ||
| print(" scripts/extract-llbc.py") |
There was a problem hiding this comment.
Name the LLBC targets in the extractor invocation.
The bare scripts/extract-llbc.py text is still too generic here; each consumer needs a different crate set, and users won’t know what to regenerate from the message alone.
pyre/check.py#L385-L387:scripts/extract-llbc.py pyre-object pyre-interpretermajit/majit-charon-reader/README.md#L57-L61:scripts/extract-llbc.py pyre-object pyre-interpretermajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rs#L40-L43:scripts/extract-llbc.py pyre-object pyre-interpreter pyre-jitmajit/majit-translate/tests/test_mir_dispatch.rs#L20-L22:scripts/extract-llbc.py pyre-interpreter
📍 Affects 4 files
pyre/check.py#L385-L387(this comment)majit/majit-charon-reader/README.md#L57-L61majit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rs#L40-L43majit/majit-translate/tests/test_mir_dispatch.rs#L20-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/check.py` around lines 385 - 387, The bare scripts/extract-llbc.py
command is too generic and doesn't specify which LLBC targets to extract, making
it unclear what users need to regenerate. At pyre/check.py (lines 385-387),
replace scripts/extract-llbc.py with scripts/extract-llbc.py pyre-object
pyre-interpreter. At majit/majit-charon-reader/README.md (lines 57-61), replace
scripts/extract-llbc.py with scripts/extract-llbc.py pyre-object
pyre-interpreter. At
majit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rs
(lines 40-43), replace scripts/extract-llbc.py with scripts/extract-llbc.py
pyre-object pyre-interpreter pyre-jit. At
majit/majit-translate/tests/test_mir_dispatch.rs (lines 20-22), replace
scripts/extract-llbc.py with scripts/extract-llbc.py pyre-interpreter.
| f'host.rustflags=["{crate_attr}"]', | ||
| ] | ||
|
|
||
| for crate in args.crates or ALL_CRATES: |
There was a problem hiding this comment.
Default crate set is inconsistent with CLI defaults.
Line 300 uses ALL_CRATES when no positional crates are passed, but main() uses DEFAULT_CRATES for --fingerprint and --list-inputs. Running scripts/extract-llbc.py with no args currently extracts more crates than the declared default behavior.
Suggested fix
- for crate in args.crates or ALL_CRATES:
+ for crate in args.crates or DEFAULT_CRATES:📝 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
| for crate in args.crates or ALL_CRATES: | |
| for crate in args.crates or DEFAULT_CRATES: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/extract-llbc.py` at line 300, The for loop at the point where crates
are iterated uses ALL_CRATES as the default fallback when no crates argument is
provided, but the main function declares DEFAULT_CRATES as the default behavior
for other command-line options. Replace ALL_CRATES with DEFAULT_CRATES in the
conditional expression to ensure consistent default crate set behavior across
all code paths, so that running the script with no arguments extracts the same
set of crates as the declared defaults for --fingerprint and --list-inputs.
Comment on lines
+95
to
+96
| with urllib.request.urlopen(url) as response, dest.open("wb") as output: | ||
| shutil.copyfileobj(response, output) |
There was a problem hiding this comment.
Add a timeout to the release download to avoid hanging CI jobs.
Line 95 opens a network connection with no timeout, so transient network stalls can block indefinitely and tie up runners.
Suggested fix
-def download(url: str, dest: Path) -> None:
- with urllib.request.urlopen(url) as response, dest.open("wb") as output:
+def download(url: str, dest: Path) -> None:
+ with urllib.request.urlopen(url, timeout=60) as response, dest.open("wb") as output:
shutil.copyfileobj(response, output)🧰 Tools
🪛 Ruff (0.15.15)
[error] 95-95: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-charon.py` around lines 95 - 96, The urllib.request.urlopen
call in the download operation does not specify a timeout parameter, which can
cause the process to hang indefinitely during network stalls. Add a timeout
parameter to the urlopen() call to prevent CI jobs from being blocked. Specify
an appropriate timeout value (in seconds) that allows sufficient time for the
release download while protecting against indefinite hangs from network issues.
Comment on lines
+170
to
+173
| with tarfile.open(archive, "r:gz") as tar: | ||
| tar.extractall(tmp_dir) | ||
| shutil.move(str(Path(tmp_dir) / "charon"), charon_dest / "charon") | ||
| shutil.move(str(Path(tmp_dir) / "charon-driver"), charon_dest / "charon-driver") |
There was a problem hiding this comment.
Unsafe archive extraction can write outside the temp directory.
Line 171 uses tar.extractall(tmp_dir) on a downloaded archive without validating member paths. A crafted tarball can perform path traversal and overwrite arbitrary files on the runner.
Suggested fix
+def safe_extract_tar_gz(archive: Path, dest: Path) -> None:
+ with tarfile.open(archive, "r:gz") as tar:
+ dest_resolved = dest.resolve()
+ for member in tar.getmembers():
+ member_path = (dest / member.name).resolve()
+ if not str(member_path).startswith(str(dest_resolved) + os.sep):
+ raise SystemExit(f"install-charon.py: unsafe tar member path: {member.name}")
+ tar.extractall(dest)
+
@@
- with tarfile.open(archive, "r:gz") as tar:
- tar.extractall(tmp_dir)
+ safe_extract_tar_gz(archive, Path(tmp_dir))🧰 Tools
🪛 Ruff (0.15.15)
[error] 171-171: Uses of tarfile.extractall()
(S202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-charon.py` around lines 170 - 173, The tar.extractall() call
on line 171 extracts archive members without validating their paths, allowing
potential path traversal attacks where a crafted tarball could write files
outside tmp_dir. Before calling tar.extractall(), iterate through all members
returned by tar.getmembers() and validate that each member's name does not
contain path traversal sequences like ".." and that the normalized path remains
within tmp_dir. Filter out or reject any members with unsafe paths before
proceeding with the extraction.
Source: Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3f5b0937
ℹ️ 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".
Comment on lines
+179
to
+181
| env: | ||
| BACKEND: ${{ matrix.backend }} | ||
| run: cargo test --all --no-default-features --features "$BACKEND" |
There was a problem hiding this comment.
In the cargo-test matrix, the Windows rows do not set shell, and GitHub documents that Windows run steps default to pwsh (with PowerShell env vars referenced as ${env:PATH}): https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#jobsjob_idstepsshell. This env: entry creates an environment variable, but PowerShell does not expand it as $BACKEND, so the Windows jobs pass an empty value to --features and fail or stop testing the selected backend; use shell: bash, ${{ matrix.backend }}, or $env:BACKEND here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install-charon.py`:
- Around line 48-92: The prepend_msvc_link function is duplicated across
multiple scripts. Create a new shared utility module to house this function,
then remove the duplicate implementations and update both script files to import
prepend_msvc_link from the new shared module. This will ensure consistent
behavior and reduce maintenance burden across the codebase.
🪄 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: 7ccf3ca8-4db2-4368-8b23-60ed95799c3d
📒 Files selected for processing (15)
.github/workflows/pyre-ci.ymlmajit/charon-corpus/README.mdmajit/majit-charon-reader/README.mdmajit/majit-charon-reader/src/lib.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rsmajit/majit-translate/tests/test_mir_dispatch.rsmajit/majit-translate/tests/test_mir_stress.rspyre/check.pypyre/pyre-jit-trace/build.rsscripts/charon-msvc-env.shscripts/extract-llbc.pyscripts/extract-llbc.shscripts/install-charon.pyscripts/install-charon.sh
💤 Files with no reviewable changes (3)
- scripts/charon-msvc-env.sh
- scripts/extract-llbc.sh
- scripts/install-charon.sh
Comment on lines
+48
to
+92
| def prepend_msvc_link(env: dict[str, str]) -> None: | ||
| if platform_info()[0] != "windows": | ||
| return | ||
| vswhere = Path( | ||
| "C:/Program Files (x86)/Microsoft Visual Studio/Installer/vswhere.exe" | ||
| ) | ||
| if not vswhere.exists(): | ||
| print( | ||
| "warn: vswhere not found; MSVC link.exe may be shadowed by Git's link.exe", | ||
| file=sys.stderr, | ||
| ) | ||
| return | ||
| try: | ||
| install = subprocess.run( | ||
| [ | ||
| str(vswhere), | ||
| "-latest", | ||
| "-products", | ||
| "*", | ||
| "-requires", | ||
| "Microsoft.VisualStudio.Component.VC.Tools.x86.x64", | ||
| "-property", | ||
| "installationPath", | ||
| ], | ||
| check=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, | ||
| text=True, | ||
| ).stdout.strip() | ||
| except subprocess.CalledProcessError: | ||
| install = "" | ||
| if not install: | ||
| return | ||
| msvc_root = Path(install) / "VC" / "Tools" / "MSVC" | ||
| candidates = sorted(msvc_root.glob("*/bin/Hostx64/x64/link.exe")) | ||
| if not candidates: | ||
| candidates = sorted(msvc_root.glob("*/bin/hostx64/x64/link.exe")) | ||
| if candidates: | ||
| env["PATH"] = str(candidates[-1].parent) + os.pathsep + env.get("PATH", "") | ||
| else: | ||
| print( | ||
| f"warn: MSVC link.exe not found under {install}; Git's link.exe may shadow it", | ||
| file=sys.stderr, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting prepend_msvc_link to a shared module.
This function is duplicated nearly verbatim in scripts/extract-llbc.py (lines 182-226). Extracting it to a shared utility module would reduce maintenance burden and ensure consistent behavior.
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 60-75: Use of unsanitized data to create processes
Context: subprocess.run(
[
str(vswhere),
"-latest",
"-products",
"*",
"-requires",
"Microsoft.VisualStudio.Component.VC.Tools.x86.x64",
"-property",
"installationPath",
],
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
text=True,
)
Note: [CWE-78].
(os-system-unsanitized-data)
[error] 60-75: Command coming from incoming request
Context: subprocess.run(
[
str(vswhere),
"-latest",
"-products",
"*",
"-requires",
"Microsoft.VisualStudio.Component.VC.Tools.x86.x64",
"-property",
"installationPath",
],
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
text=True,
)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.15)
[error] 61-61: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-charon.py` around lines 48 - 92, The prepend_msvc_link
function is duplicated across multiple scripts. Create a new shared utility
module to house this function, then remove the duplicate implementations and
update both script files to import prepend_msvc_link from the new shared module.
This will ensure consistent behavior and reduce maintenance burden across the
codebase.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 984dd089e6
ℹ️ 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".
| # extract-llbc.py also self-skips by fingerprint; this gate avoids even | ||
| # checking per-crate stamps when the CI cache restored the whole set. | ||
| if: steps.llbc-cache.outputs.cache-hit != 'true' | ||
| run: scripts/extract-llbc.py pyre-object pyre-interpreter pyre-jit |
There was a problem hiding this comment.
On the windows-latest leg this step no longer pins shell: bash; GitHub documents unspecified Windows run steps as PowerShell (pwsh -command), unlike the previous extractor step. On an LLBC cache miss the prepare job now relies on Windows .py file association/PATHEXT instead of the script shebang used by bash, so the extractor can fail before the cache is saved and every dependent job then fails; keep this step under shell: bash or invoke python scripts/extract-llbc.py ... explicitly.
Useful? React with 👍 / 👎.
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 (1)
.github/workflows/pyre-ci.yml (1)
47-65:⚠️ Potential issue | 🟠 MajorSplit
prepare-charon-llbcper-OS or move preparation into each downstream job to isolate failures.
prepare-charon-llbcis a single matrix job over three OSes. When any matrix leg fails,needs.prepare-charon-llbc.resultbecomes 'failure', causing the guard steps in bothcargo-test(6 matrix combinations) andpyre-check(3 matrix combinations) to fail immediately. A flaky Windows source build will suppress test signal for Linux and macOS even when their caches were prepared successfully. Split preparation into per-OS jobs, or move preparation back into each downstream OS job so failures stay isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pyre-ci.yml around lines 47 - 65, The prepare-charon-llbc job uses a single matrix job across three OSes, causing the entire job to fail if any OS leg fails and blocking all downstream jobs (cargo-test and pyre-check) regardless of their OS requirements. To isolate failures by OS, split the prepare-charon-llbc job into three separate jobs—one for each OS (ubuntu-24.04, macos-latest, and windows-latest)—each with its own name and without a matrix strategy, or alternatively move the Charon/LLBC preparation logic directly into the downstream job definitions (cargo-test and pyre-check) for each respective OS so they prepare their own dependencies. Update the needs references in downstream jobs to point to the appropriate per-OS preparation job(s) or remove the prepare-charon-llbc dependency entirely if preparation is moved inline. This ensures that a failure in one OS preparation does not block tests for other OSes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pyre-ci.yml:
- Around line 55-59: Move the CHARON_VERSION environment variable definition
from the job-specific env section (currently shown at line 59) to the
workflow-level env section at the top of the file so it becomes a true single
source of truth. Then remove the duplicate CHARON_VERSION definitions from the
cargo-test and pyre-check jobs so they inherit the workflow-level variable
instead. This ensures that a single pin bump updates the version consistently
across all jobs and cache lookups.
In `@scripts/extract-llbc.py`:
- Around line 45-49: The current implementation in the pyre-module branch passes
all features with a single "pyre-interpreter/" prefix, but when cargo_features
contains multiple comma-separated values, Cargo will interpret this incorrectly.
Instead, you need to prefix each individual feature with "pyre-interpreter/"
separately. Split the cargo_features string by commas, add the
"pyre-interpreter/" prefix to each feature individually, then rejoin them with
commas before passing to the --features argument. This ensures that a
comma-separated list like "a,b" becomes "pyre-interpreter/a,pyre-interpreter/b"
rather than "pyre-interpreter/a,b".
In `@scripts/install-charon.py`:
- Around line 100-115: Replace the version-based identity tracking with an
immutable artifact identity across all three sites. In scripts/install-charon.py
at lines 100-115, compute an immutable identity (such as a hash or checksum) of
the downloaded or built Charon binary, write this identity to the
.installed-version file instead of just the version string, and update the
comparison logic to check if the stored immutable identity matches the current
one before skipping reinstallation. In scripts/extract-llbc.py at lines 247-249,
modify the code that reads .installed-version to interpret it as the immutable
tool identity rather than a version string only. In scripts/extract-llbc.py at
lines 252-269, update the stamp_for() function to include the immutable tool
identity from .installed-version so that LLBC regeneration is triggered when the
actual installed Charon binary changes, not just when the version label changes.
---
Outside diff comments:
In @.github/workflows/pyre-ci.yml:
- Around line 47-65: The prepare-charon-llbc job uses a single matrix job across
three OSes, causing the entire job to fail if any OS leg fails and blocking all
downstream jobs (cargo-test and pyre-check) regardless of their OS requirements.
To isolate failures by OS, split the prepare-charon-llbc job into three separate
jobs—one for each OS (ubuntu-24.04, macos-latest, and windows-latest)—each with
its own name and without a matrix strategy, or alternatively move the
Charon/LLBC preparation logic directly into the downstream job definitions
(cargo-test and pyre-check) for each respective OS so they prepare their own
dependencies. Update the needs references in downstream jobs to point to the
appropriate per-OS preparation job(s) or remove the prepare-charon-llbc
dependency entirely if preparation is moved inline. This ensures that a failure
in one OS preparation does not block tests for other OSes.
🪄 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: bd0cda1e-b74f-4d8b-acfd-532ced85192e
📒 Files selected for processing (15)
.github/workflows/pyre-ci.ymlmajit/charon-corpus/README.mdmajit/majit-charon-reader/README.mdmajit/majit-charon-reader/src/lib.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rsmajit/majit-translate/tests/test_mir_dispatch.rsmajit/majit-translate/tests/test_mir_stress.rspyre/check.pypyre/pyre-jit-trace/build.rsscripts/charon-msvc-env.shscripts/extract-llbc.pyscripts/extract-llbc.shscripts/install-charon.pyscripts/install-charon.sh
💤 Files with no reviewable changes (3)
- scripts/install-charon.sh
- scripts/charon-msvc-env.sh
- scripts/extract-llbc.sh
Comment on lines
55
to
59
| # Single source of truth for the pinned charon tag. MUST mirror | ||
| # CHARON_VERSION_DEFAULT in scripts/install-charon.sh: it drives both | ||
| # the cache keys below AND install-charon.sh's .installed-version | ||
| # CHARON_VERSION_DEFAULT in scripts/install-charon.py: it drives both | ||
| # the cache keys below AND install-charon.py's .installed-version | ||
| # stamp, so a pin bump here misses the cache and re-installs. | ||
| CHARON_VERSION: nightly-2026.05.29 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
This pin still isn't a true single source of truth.
CHARON_VERSION is redefined again in cargo-test and pyre-check, so the next pin bump can update Line 59 but miss one restore job and turn its cache lookup into a guaranteed miss. Hoist the pin to a workflow-level env: and let all three jobs inherit it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pyre-ci.yml around lines 55 - 59, Move the CHARON_VERSION
environment variable definition from the job-specific env section (currently
shown at line 59) to the workflow-level env section at the top of the file so it
becomes a true single source of truth. Then remove the duplicate CHARON_VERSION
definitions from the cargo-test and pyre-check jobs so they inherit the
workflow-level variable instead. This ensures that a single pin bump updates the
version consistently across all jobs and cache lookups.
Comment on lines
+45
to
+49
| if crate == "pyre-module": | ||
| return ( | ||
| root / "pyre" / "pyre-module", | ||
| ["--features", f"pyre-interpreter/{cargo_features}"], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cargo, how is --features pyre-interpreter/a,bparsed for dependency features? Does thepyre-interpreter/prefix apply only toa, or to both aandb?
💡 Result:
In Cargo, the --features flag parses the comma-separated list of arguments individually [1][2]. When using the syntax package-name/feature-name, the prefix applies strictly to the feature immediately following the slash [1][2]. In the case of --features pyre-interpreter/a,b, the input is parsed as two distinct feature requests: 1. pyre-interpreter/a (enables feature 'a' on the dependency 'pyre-interpreter') [1][2] 2. b (enables feature 'b' on the current package/crate being compiled) [1][2] The pyre-interpreter/ prefix does not apply to b. To enable both features on the same dependency, you must explicitly include the prefix for each, such as: --features pyre-interpreter/a,pyre-interpreter/b [1][2].
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/features.html
- 2: https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/features.md
🏁 Script executed:
# Read the file around lines 45-49 to verify the snippet
sed -n '35,60p' scripts/extract-llbc.pyRepository: youknowone/pyre
Length of output: 1089
🏁 Script executed:
# Search for how cargo_features is passed and used
rg "cargo_features" scripts/extract-llbc.py -B 2 -A 2Repository: youknowone/pyre
Length of output: 2750
🏁 Script executed:
# Search for CARGO_FEATURES usage and how it's set
rg "CARGO_FEATURES" scripts/extract-llbc.py -B 2 -A 2Repository: youknowone/pyre
Length of output: 491
🏁 Script executed:
# Check if there are tests or examples showing multi-feature usage
rg "CARGO_FEATURES" . --include="*.py" --include="*.sh" -B 1 -A 1 | head -50Repository: youknowone/pyre
Length of output: 501
Prefix each forwarded feature for pyre-module.
When CARGO_FEATURES contains multiple entries, this builds --features pyre-interpreter/a,b, which Cargo parses as pyre-interpreter/a plus bare b. metadata() already prefixes each feature individually, so multi-feature runs can fingerprint one dependency set and extract another.
Suggested fix
if crate == "pyre-module":
+ forwarded = [
+ f"pyre-interpreter/{feature.strip()}"
+ for feature in cargo_features.split(",")
+ if feature.strip()
+ ]
return (
root / "pyre" / "pyre-module",
- ["--features", f"pyre-interpreter/{cargo_features}"],
+ ["--features", ",".join(forwarded)] if forwarded else [],
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/extract-llbc.py` around lines 45 - 49, The current implementation in
the pyre-module branch passes all features with a single "pyre-interpreter/"
prefix, but when cargo_features contains multiple comma-separated values, Cargo
will interpret this incorrectly. Instead, you need to prefix each individual
feature with "pyre-interpreter/" separately. Split the cargo_features string by
commas, add the "pyre-interpreter/" prefix to each feature individually, then
rejoin them with commas before passing to the --features argument. This ensures
that a comma-separated list like "a,b" becomes
"pyre-interpreter/a,pyre-interpreter/b" rather than "pyre-interpreter/a,b".
Comment on lines
+100
to
+115
| version = os.environ.get("CHARON_VERSION", CHARON_VERSION_DEFAULT) | ||
| root = repo_root() | ||
| shared = Path(os.environ.get("PYRE_SHARED_BUILD", root.parent / ".pyre-build")) | ||
| platform_key, asset, exe, from_source = platform_info() | ||
| charon_dest = Path( | ||
| os.environ.get("CHARON_DEST", shared / "charon" / platform_key) | ||
| ) | ||
|
|
||
| stamp = charon_dest / ".installed-version" | ||
| charon_bin = charon_dest / f"charon{exe}" | ||
| if charon_bin.exists() and stamp.exists(): | ||
| current = stamp.read_text().strip() | ||
| if current == version: | ||
| print(f"charon {version} already installed at {charon_dest}") | ||
| return | ||
| print(f"charon at {charon_dest} is {current}; replacing with {version}") |
There was a problem hiding this comment.
Use an immutable Charon identity across installation and LLBC stamping. Both scripts currently key caching off the version label written to .installed-version, but that label is not the actual artifact identity. If the same label resolves to different binaries, the installer will skip replacement and the extractor will reuse stale fingerprints.
scripts/install-charon.py#L100-L115: write an immutable identity to.installed-versionand only skip reinstall when that exact identity matches.scripts/extract-llbc.py#L247-L249: stop treating.installed-versionas a version string only; read the immutable tool identity instead.scripts/extract-llbc.py#L252-L269: include that immutable identity instamp_for()so LLBC regeneration follows the actual installed Charon bits.
📍 Affects 2 files
scripts/install-charon.py#L100-L115(this comment)scripts/extract-llbc.py#L247-L249scripts/extract-llbc.py#L252-L269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-charon.py` around lines 100 - 115, Replace the version-based
identity tracking with an immutable artifact identity across all three sites. In
scripts/install-charon.py at lines 100-115, compute an immutable identity (such
as a hash or checksum) of the downloaded or built Charon binary, write this
identity to the .installed-version file instead of just the version string, and
update the comparison logic to check if the stored immutable identity matches
the current one before skipping reinstallation. In scripts/extract-llbc.py at
lines 247-249, modify the code that reads .installed-version to interpret it as
the immutable tool identity rather than a version string only. In
scripts/extract-llbc.py at lines 252-269, update the stamp_for() function to
include the immutable tool identity from .installed-version so that LLBC
regeneration is triggered when the actual installed Charon binary changes, not
just when the version label changes.
Owner
Author
|
@copilot what seems to be wrong? |
Contributor
The only remaining failures are This is a pre-existing JIT bug on The |
youknowone
added a commit
that referenced
this pull request
Jun 25, 2026
Convert the 23 const/none Operand::from_boxref(&BoxRef::from_opref(X)) round-trips to Operand::from_opref(X) across history, compile, unroll, info, vstring, dependency. Const X resolves to Operand::Const and OpRef::NONE to Operand::None either way, so behavior is unchanged. The 6 position-only sites (guard.rs args, info.rs eq_pos) keep the wrapper pending producer binding (#175). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jun 25, 2026
Convert the 23 const/none Operand::from_boxref(&BoxRef::from_opref(X)) round-trips to Operand::from_opref(X) across history, compile, unroll, info, vstring, dependency. Const X resolves to Operand::Const and OpRef::NONE to Operand::None either way, so behavior is unchanged. The 6 position-only sites (guard.rs args, info.rs eq_pos) keep the wrapper pending producer binding (#175). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jun 25, 2026
* ir: re-home forwarding accessors onto Op/InputArg Add a ForwardingHost trait carrying get_forwarded, the four set_forwarded_* setters, clear_forwarded and the ptr_info/int_bound projections; impl it for Op and InputArg, reading their forwarded slot directly. BoxRef's same-named methods now delegate through bound_op()/bound_inputarg() via read_forwarding_host / with_forwarding_host. The set_forwarded_* self-cycle assert (resoperation.py:241) re-homes onto the host via std::ptr::eq(self, Rc::as_ptr(target)), since an Op-side &self cannot obtain its containing Rc. Add Operand::get_box_replacement as the canonical _forwarded chain walker; BoxRef::get_box_replacement delegates to it, keeping the identity result for the position-only/None case that Operand::from_boxref rejects. Assisted-by: Claude * metainterp: forward superseded stand-in off producer Op The two replace_op_with supersession sites built a throwaway BoxRef::from_bound_op(&producer) only to call set_forwarded_op on it. Call the ForwardingHost trait method on the producer OpRc directly and re-export ForwardingHost from majit-metainterp's box module. Assisted-by: Claude * ir: dispatch Operand::same_box natively same_box matched on the operand union directly (Rc::ptr_eq for Op/InputArg, bit-exact Value compare for Const, None matches itself) instead of round-tripping through to_boxref().same_box(). Add a test covering the InputArg, float-Const (0.0 vs -0.0, NaN == NaN), and cross-kind arms. Assisted-by: Claude * metainterp: build Operand directly at bound-producer round-trips Four sites minted a BoxRef::from_bound_op / from_bound_inputarg only to immediately Operand::from_boxref it. Call Operand::from_bound_op / from_bound_inputarg directly (pyjitpl Label-arg builders x3, schedule rebind-arg). Behaviour-identical: both yield Operand::Op / Operand::InputArg over the same producer Rc. Assisted-by: Claude * ir: add Operand::from_opref; build it in history bind_remapped Operand::from_opref maps an OpRef's absent/inline-const arms to Operand::None / Operand::Const natively (mirror of BoxRef::from_opref's non-position cases) and panics on a position-only ref, the same #9 invariant tripwire as Operand::from_boxref. history cut_trace's bind_remapped now returns Operand directly (from_opref for none/const, from_bound_op / from_bound_inputarg for producers), so the two re-emit consumers drop their Operand::from_boxref wrappers. BoxRef's import moves into the test module, its sole remaining user here. Assisted-by: Claude * metainterp: drop const/none from_boxref round-trips Convert the 23 const/none Operand::from_boxref(&BoxRef::from_opref(X)) round-trips to Operand::from_opref(X) across history, compile, unroll, info, vstring, dependency. Const X resolves to Operand::Const and OpRef::NONE to Operand::None either way, so behavior is unchanged. The 6 position-only sites (guard.rs args, info.rs eq_pos) keep the wrapper pending producer binding (#175). Assisted-by: Claude * ir: add Operand forwarding accessors mirroring BoxRef Add bound_op / bound_inputarg / get_forwarded / set_forwarded_{op, inputarg,const,info} / clear_forwarded / ptr_info{,_handle,_mut} / int_bound{,_handle,_mut} to Operand, delegating to the carried Op / InputArg ForwardingHost (Const / None take the reader default and reject forwarding writes). The operand IS the producer Rc, so no Weak upgrade and no box_cache. This is the successor surface to the BoxRef delegation methods, prerequisite for flipping the optimizer's forwarding writers (make_equal_to / make_constant) off BoxRef. Assisted-by: Claude * optimizeopt: flip make_constant_box to Operand make_constant_box now takes &Operand and uses the Operand forwarding accessors (int_bound_mut/set_forwarded_const). Add materialize_operand_at and route make_constant_box feeders through it; make_constant_arg and optimizer.rs bridge via from_boxref at their BoxRef boundaries (retire when those callers flip). Assisted-by: Claude * optimizeopt: flip intbounds helper params to Operand Flip the BoxRef params of resolve_box, getintbound_arg, find_producing_op, propagate_bounds_backward, make_int_lt/le/gt/ge, make_unsigned_lt/le/gt/ge, make_eq and make_ne to &Operand. External callers drop the .to_boxref() bridge and pass op.arg(N) operands directly. resolve_box and getintbound_arg keep their BoxRef return so the getintbound_b/with_intbound_mut/getintbound_handle sinks fed by their results are untouched; they convert to BoxRef internally. Drains 254 to_boxref call sites (intbounds.rs 257->6). Assisted-by: Claude * optimizeopt: resolve op-args via Operand twin resolvers Add resolve_operand_box / resolve_operand_box_opt: Operand-input siblings of resolve_box_box / resolve_box_box_opt that keep the BoxRef return and convert internally. Migrate the 176 drain-form callers (resolve_box_box(&X.to_boxref()) where X is op.arg(i) or an Operand local) to the twins, dropping the .to_boxref() bridge. Drains 171 to_boxref (metainterp 329->158); from_boxref unchanged. The BoxRef-local (boxsrc) callers stay on the original resolvers. Assisted-by: Claude * metainterp: drain fixture from_boxref via Operand fixtures Add rooted_resop_operand / rooted_inputarg_operand test fixtures that shed the synthetic producer to Operand::Op / Operand::InputArg directly, rooting it in the same thread-local pool as the BoxRef forms so a position-only re-resolution after the Operand drops stays bound. Migrate the 655 Operand::from_boxref(&rooted_*_box(..)) round-trips (single- and multi-line) to the new fixtures and update each test module's import accordingly. metainterp from_boxref 1001->349. The from_boxref(&match{..}) fixture wraps in vector/heap are left as-is. Assisted-by: Claude * optimizeopt: route info.rs guard/residual args via Operand make_guards and the force-residualization emitter built their guard / residual op arguments as BoxRef locals (materialize_box_at / resolve_box_box / the alloc_const closure) and wrapped each in Operand::from_boxref at the Op::new call. Source those locals through the Operand-returning siblings instead: materialize_operand_at, the new resolve_box_operand twin, and alloc_const returning Operand. The args are then passed by clone. The position-only RawBuffer guard operand (eq op living in short, not in ctx's registries) stays on from_boxref(&BoxRef::from_opref(..)) pending E5b producer-binding. info.rs production from_boxref 50->1; metainterp total 349->304. Assisted-by: Claude * optimizeopt: route int op-emission args via Operand intdiv / intutils / rewrite built residual arithmetic op arguments as BoxRef locals from materialize_box_at, wrapped in Operand::from_boxref at Op::new. Source those locals through materialize_operand_at and pass by clone. rewrite's source_box keeps a BoxRef binding (it is also consumed as a position handle, not only an op argument) and is left untouched. metainterp from_boxref 304->271. Assisted-by: Claude * optimizeopt: route vstring/virtualstate op args via Operand vstring string-residual emitters and virtualstate guard emitters built op arguments as BoxRef locals (materialize_box_at / resolve_box_box) wrapped in Operand::from_boxref at Op::new. Source the op-argument-only locals through materialize_operand_at / resolve_box_operand and pass by clone. Locals also consumed as BoxRef elsewhere (vstring op/source/srcbox/ lengthbox/..., virtualstate box_) keep their BoxRef binding. metainterp from_boxref 271->242. Assisted-by: Claude * optimizeopt: collapse residual op-arg from_boxref guard.rs const-compare operands collapse the Operand::from_boxref(&BoxRef::from_opref(x)) round-trip to the equivalent Operand::from_opref(x) (the renamer arg, which is not const- namespace, is left for the structural phase). optimizer label/same-as emission and virtualize vref/null guard emission source their op arguments through materialize_operand_at instead of from_boxref of a BoxRef local. metainterp from_boxref 242->232. Assisted-by: Claude * optimizeopt: collapse resolve_operand_box rewrap round-trips Add resolve_operand_operand: resolve an operand to its forwarded terminal and return it as an Operand, so the seven Operand::from_boxref(&resolve_operand_box(..)) resolve-then-rewrap round-trips at Op::new arg sites (heap.rs 5, rewrite.rs 2) collapse to a single resolver call. The internal from_boxref retires when resolve_box_box itself returns an Operand. Assisted-by: Claude * optimizeopt: native resolve_operand_operand resolver Rewrite resolve_operand_operand to resolve an operand to its _forwarded terminal natively on the Operand carrier, dropping the Operand::from_boxref(&resolve_operand_box(..)) resolve-then-rewrap body. Add two native helpers it consumes: - resolve_to_operand: Operand mirror of resolve_to_boxref (producer Op / inputarg_refs / inline-Const, else None). - get_box_replacement_operand: total Operand mirror of get_box_replacement; position-only fallback routes through Operand::from_opref, which panics (the armed position-only tripwire) since Operand cannot carry one. A #[cfg(debug_assertions)] tripwire asserts the native walk agrees with the legacy resolve-then-rewrap. Gate: majit-ir 332, dynasm 1333, cranelift 1331 lib; check.py 139/139 both backends. Assisted-by: Claude * optimizeopt: native Option<Operand> resolvers drain emit-path from_boxref Add three native Option<Operand> resolver siblings: - get_box_replacement_operand_opt (mirror get_box_replacement_box) - resolve_operand_operand_opt (mirror resolve_operand_box_opt) - get_box_replacement_not_const_operand (mirror get_box_replacement_not_const_box) Each resolves on the Operand carrier without minting a BoxRef (debug tripwire asserts agreement with the legacy resolve-then-rewrap). Migrate the emit-path consumers off resolve_*_box + from_boxref: optimizer.rs preamble setarg / fail_args, send_extra_operation arg loop, emit_operation force_box loop, finalize fail_args not-const resolve; plus shortpreamble getarrayitem arg mint and pure.rs test fixtures. These production arg/fail-arg paths no longer create a BoxRef per operand. from_boxref 254->244, to_boxref 161->160. Gate: majit-ir 332, dynasm 1333, cranelift 1331 lib; check.py 139/139 both backends. Assisted-by: Claude * optimizeopt: native setarg-loop resolve in heap/virtualize/rewrite fixtures Migrate the 10 arg-canonicalization setarg loops (8 heap test fixtures + virtualize canonicalize_args + rewrite resolve_op_args_in_ctx) off the resolve_operand_box_opt + Operand::from_boxref idiom onto native resolve_operand_operand_opt / materialize_operand_at, so each arg resolves on the Operand carrier without a BoxRef round-trip. from_boxref 244->224. Gate: majit-ir 332, dynasm 1333, cranelift 1331 lib; check.py 139/139 both backends. Assisted-by: Claude * optimizeopt: native Operand keys for heapcache escape tracking Re-key the heapcache escape-tracking maps (unescaped VecSet, heapc_deps VecAssoc — both already Operand-typed) off the from_boxref(&resolve_box) bridge. Flip escape_box / is_unescaped to take &Operand and resolve every key through the native get_box_replacement_operand_opt / resolve_operand_operand_opt: escape_from_write value/container, the arglist escape loop (now over op.arg(i), dropping the getarglist BoxRef materialization), the heapc_deps owner lookup, the flush-owner escaped check, and the two malloc unescaped inserts. All keys remain the canonical producer Operand (ptr_eq-identical to the prior from_boxref keys), so set membership is unchanged. heap.rs from_boxref 24->8. Gate: majit-ir 332, dynasm 1333, cranelift 1331 lib; check.py 139/139 both backends. Assisted-by: Claude * optimizeopt: materialize_operand_at at op-construction arg sites Drop the Operand::from_boxref(&ctx.materialize_box_at(opref)) round-trip at op-building arg sites in favor of materialize_operand_at, which mints the canonical bound host directly as an Operand: heap getfield/getarrayitem replay, virtualstate GuardNonnull, unroll extra-live label args, pure postponed-op force. from_boxref 217->210. Gate: majit-ir 332, dynasm 1333, cranelift 1331 lib; check.py 139/139 both backends. Assisted-by: Claude * optimizeopt: flip OptContext readers to &Operand args Flip the OptContext reader/writer family (~70 methods across optimizeopt: mod/info/heap/pure/virtualize/vstring/intbounds/unroll/...) to accept &Operand instead of &BoxRef. The set is irreducibly atomic — the methods share box-locals, so the signatures move together. Add OptContext::operand_of_box as the panic-free position-only bridge: bound/const boxes route through Operand::from_boxref, position-only boxes materialize via materialize_operand_at (never returns a position-only operand). Add Operand::get_value mirroring BoxRef::get_value. In &self export paths (unroll expand_info) where operand_of_box (&mut) is unavailable, read ptr_info directly off the BoxRef (get_box_replacement(false).ptr_info()), which is self-independent. Assisted-by: Claude * optimizeopt: carry VString slice/concat fields as Operand Flip VStringSliceInfo {s,start,lgtop} and VStringConcatInfo {vleft,vright} (ptr_info.rs) from BoxRef to Operand. These fields hold bound resolver results (op-args resolve to producers), so the position-only from_opref hazard is eliminated at the source rather than patched at each read. Construction paths switch resolve_operand_box -> resolve_operand_operand (the native walk carries an armed migration tripwire asserting equality with the resolve-then-rewrap legacy path, gate-green with debug_assertions on), draining the from_boxref/to_boxref read bridges in vstring/info. The inherited StrPtrInfo.lgtop (Option<BoxRef>) stays BoxRef; str_child_oprefs and the GC-relocate visitor are untouched (the visitor never walks Str). Test fixtures constructing these infos move to materialize_operand_at so they bind a producer instead of a position-only box. Assisted-by: Claude * optimizeopt: carry VStringPlainInfo _chars as Operand Flip VStringPlainInfo._chars (ptr_info.rs) from Vec<Option<BoxRef>> to Vec<Option<Operand>>, completing the VStringVariant BoxRef teardown (Plain/Slice/Concat fields all carry Operand now; only the inherited StrPtrInfo.lgtop stays BoxRef). The two STRSETITEM/string-copy char-store sites write inside a with_plain_info_mut closure that holds ctx, so the char position is materialized to a bound producer (materialize_operand_at) before the closure rather than stored as a position-only from_opref box. Readers already used to_opref / const_int / walk_const_ptr_refs, which Operand mirrors; the Plain const-spec read switches resolve_box_box_opt -> resolve_operand_box_opt. Test fixtures move to materialize_operand_at. Assisted-by: Claude * optimizeopt: carry StrPtrInfo.lgtop as Operand Flip the inherited StrPtrInfo.lgtop (ptr_info.rs) from Option<BoxRef> to Option<Operand>, completing the StrPtrInfo BoxRef teardown (every Str PtrInfo field now carries Operand). set_str_lgtop takes &mut self and materializes the length op position to its bound producer (materialize_operand_at) before the cache write, so the field never holds a position-only from_opref box; the four call sites are already in &mut self optimizer methods. The getstrlen reader switches resolve_box_box_opt -> resolve_operand_box_opt; the newstr writer reuses the already-materialized arg_length. Slice/concat construction and the set_vstring_slice test helper drop their Some(_.to_boxref()) bridges; test fixtures move to materialize_operand_at. Assisted-by: Claude * optimizeopt: carry VirtualRawSliceInfo.parent as Operand Assisted-by: Claude * optimizeopt: carry virtual field families as Operand Flip VirtualInfo.fields, VirtualArrayInfo.items, VirtualStructInfo.fields and VirtualArrayStructInfo.element_fields from Vec<.. BoxRef> to Operand, forced together by the shared force_at_the_end_of_preamble closure (FnMut(Operand) -> Operand). Virtual writer arms store the Operand directly; the Instance/Struct/Array cache arms bridge transitionally via to_boxref into FieldEntry::Value. Metainterp callers materialize the field value at the caller before the closure borrow. Assisted-by: Claude * optimizeopt: carry Virtualizable field state as Operand Flip VirtualizableFieldState.fields and .arrays from Vec<.. BoxRef> to Operand, removing the transitional set_field_boxref bridge so the virtualizable static/array slots reuse set_field / set_array_element on Operand. Writers materialize the slot value at the caller before storing; the init seed and mirror_setarrayitem materialize before the with_ptr_info_mut borrow. Readers (walk_const_ptr_refs, visitor_walk_recursive, tracked_array_element) are unchanged. Assisted-by: Claude * optimizeopt: carry FieldEntry::Value as Operand Flip FieldEntry::Value(BoxRef) to FieldEntry::Value(Operand), dropping the transitional to_boxref bridges in the Instance/Struct/Array setfield/setitem cache arms and the Virtual-family all_items/getfield/getitem projections. as_seen_box bridges the Value arm via to_boxref so the export/import box-identity keying is preserved: a bound operand resolves to its canonical box (memoized on the producer) so export and import return the same Rc, while a Const operand carries no exported info and never enters the ptr_eq map. heap.rs getfield_from_cache materializes the lazy-set RHS at the caller (panic-free, position-preserving). as_seen_opref and the FieldEntry::Value(cached) match arms read through Operand unchanged. Assisted-by: Claude * optimizeopt: rehome box refs to history after #259 rebase Adapt box-pool's BoxRef-teardown references to origin/main's #259 module rename, and finish two conflict resolutions left incomplete during the rebase: - crate::r#box::test_support -> crate::history::test_support - crate::r#box::{BoxRef,Forwarded} -> majit_ir::box_ref::* - VirtualRawSliceInfo/VirtualRawBufferInfo -> RawSlicePtrInfo/RawBufferPtrInfo - vstring string_copy_parts: match the pre-extracted Action enum (the rebase left a stale match on a removed `variant` binding) - history.rs tests: drop leftover conflict markers + duplicate imports - mod.rs: drop duplicate Forwarded imports - dependency.rs: import BoxRef Assisted-by: Claude * optimizeopt: drain from_boxref via resolve_operand_operand Migrate 164 production resolve_operand_box/_opt value-consumer call sites to the resolve_operand_operand/_opt Operand twins, collapsing 153 Operand::from_boxref bridges (871 -> 718). The twins carry a live debug_assert tripwire asserting native walk == legacy resolve-then- rewrap byte-for-byte across the corpus, so the flip is value-identical. - resolve_array_source return flipped (BoxRef,u32) -> (Operand,u32); vstring (box,index) carriers flipped with their in-file consumers. - Position-only reads `resolve_operand_box(x).to_opref()` kept on the box primitive: they materialize no value and tolerate dangling test positions (the Operand form would panic via from_opref). - 2 sites kept on box form: EnsuredPtrInfo::ForwardedBox(BoxRef) variant (mod.rs) and add_preamble_op_from_pop's BoxRef param (earlyforce.rs, #173 peel boundary). - resolve_operand_box/_opt wrappers retained to back the tripwire; retired with the tripwire in a follow-up slice. Assisted-by: Claude * optimizeopt: route resolve_box_box operand args via twin Migrate 25 resolve_box_box/_opt(&x.to_boxref()) sites (x already an Operand) to the resolve_operand_operand/_opt twins, dropping 25 .to_boxref() input bridges and ~29 Operand::from_boxref output wraps (from_boxref 718 -> 689). resolve_operand_box(&x) is defined as resolve_box_box(&x.to_boxref()), so the live tripwire proves the flip value-identical. - make_constant_arg: or_else materialize_box_at -> materialize_operand_at so the Option unifies on Operand. - Held genuine &BoxRef sinks: PtrInfo::force_box (BoxRef by value) in vstring/pure; getarglist / preamble_op.op container sites in heap/mod (#173 peel boundary); the resolve_operand_box/_opt definitions. Assisted-by: Claude * optimizeopt: arm get_box_replacement_operand_opt tripwire Add a debug_assert tripwire to get_box_replacement_operand_opt asserting its native Operand walk agrees (presence + identity) with the BoxRef-form get_box_replacement_box. Direct get_box_replacement_box callers do not flow through the resolve_operand_operand_opt tripwire, so this covers them before migrating those callers to the Operand twin. Silent across lib tests + check.py 144/144 both backends. Assisted-by: Claude * pyre-jit-trace: access call_descr effect-infos via re-export #270 made majit_metainterp::call_descr a pub(crate) module while re-exporting its effect-info constants at the crate root. Route the four cross-crate accesses (ELIDABLE_CANNOT_RAISE_EFFECT_INFO, cannot_raise_effect_info) through the crate-root re-export instead of the now-private module path. Latent since the rebase, masked by cargo reusing a stale pyre-jit-trace build until a metainterp change forced its rebuild. Assisted-by: Claude * optimizeopt: migrate get_box_replacement_box to operand twin Replace get_box_replacement_box callers with the tripwire-proven get_box_replacement_operand_opt twin across 8 fully-migrated files plus partial mod/optimizer/heap, collapsing 135 Operand::from_boxref bridges (from_boxref 689 -> 554). The armed equivalence tripwire proves each flip value-identical (silent across lib tests + check.py 144/144 both backends). - Excludes the #173 export-boundary files (unroll/shortpreamble/ virtualstate) where resolved boxes key the const-ptr_eq exported_infos. - Held genuine BoxRef sinks: SHAPE A position-reads (b.to_opref(), would panic via from_opref on a position-only fixture), force_box(BoxRef by value), force_child's BoxRef return. - mod/optimizer/heap partially migrated (subagent limit mid-run); ~47 get_box_replacement_box callers remain there for a follow-up slice. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jun 28, 2026
…ion (#297) * optimizeopt: restore object-native operand resolution Resolve op-arg operands through resolve_operand_operand (which walks the operand's own get_box_replacement(false) chain before deferring to the OpRef store) instead of get_replacement_opref on the collapsed OpRef position, matching get_box_replacement(op.getarg(i)). Sites: heap.rs, rewrite.rs, virtualize.rs, optimizer.rs, info.rs, and mod.rs (maybe_replace_guard_value, structinfo_setfield, ensure_ptr_info_arg0). mod.rs force_op_from_preamble_op and resolved_for_pop resolve preamble_op.op via BoxRef::get_box_replacement(false) directly. mod.rs get_box_replacement_operand returns Operand::bound_from_opref so the OpRef-store fallback carries a bound producer (total mirror of BoxRef::get_box_replacement). runtime_value_of routes through resolve_to_operand. guard.rs/schedule.rs/vector.rs: document the #175 vectorizer and guard operand-binding adaptations; correct the stale mark_guard comment (CompileLoopVersionDescr exists at compile.rs). Assisted-by: Claude * optimizeopt: flip store_final_boxes to Vec<Operand> store_final_boxes (resoperation.rs) now takes Vec<Operand> and writes the fail_args slot — already SmallVec<[Operand;3]> — without the per-element Operand::from_boxref conversion; the debug duplicate check uses Operand::same_box. The store_final_boxes_in_guard caller (mod.rs) builds the live operands via resolve_to_operand, falling back to Operand::from_opref on a producerless position (the same contract the former from_boxref step enforced: NONE -> Operand::None, Const -> Operand::Const, producerless non-Const -> panic). This drops the resolve_to_boxref + BoxRef::from_opref hop, leaving get_box_replacement_impl as the sole resolve_to_boxref caller. The majit-gc test callers build fail args through the existing bound_operand_from_opref (ro) helper; the bound_box_from_opref (rb) import is removed. Assisted-by: Claude * optimizeopt: delete resolve_to_boxref; Operand-native chain walk get_box_replacement_impl now walks the forwarding chain in Operand space: resolve_to_operand + Operand::get_box_replacement + a new operand_to_opref reconstructor, replacing the resolve_to_boxref + BoxRef::get_box_replacement + box_to_opref round-trip. operand_to_opref mirrors box_to_opref arm for arm — Const reconstructs its inline OpRef via Operand::Const::to_opref, a position-bearing terminal rebuilds input_arg_typed/op_typed, and a Type::Void phantom preserves the source variant via with_raw. With their last callers gone, resolve_to_boxref and box_to_opref are deleted. Stale doc/comment references to both (and to the already-removed get_box_replacement_box) repoint to resolve_to_operand / operand_to_opref / get_box_replacement_operand. Assisted-by: Claude * optimizeopt: flip box_for_operand/mint_box_at to Operand Drain the remaining production-resolver from_boxref wraps: - recorder.rs box_for_operand returns Operand directly; box_args drops the Operand::from_boxref re-wrap. The #[cfg(test)] position-only arms bind a synthetic producer via Operand::bound_from_opref (to_opref-identical) instead of returning a position-only BoxRef that from_boxref would panic on. cfg(not(test)) arms keep the dense-producer panic. - guard.rs GuardStrengthenOpt::rename_op setarg passes Operand::from_opref(replacement) (byte-exact collapse of Operand::from_boxref(&BoxRef::from_opref(...))); drop the now unused BoxRef import. - mod.rs mint_box_at returns Operand (from_bound_op); reserve_virtual_box consumes it without the from_boxref wrap. Assisted-by: Claude * optimizeopt: re-home heal_arg_to_canonical onto Operand heal_arg_to_canonical takes &Operand and reads/writes forwarding through the operand's own producer (bound_op / get_forwarded / set_forwarded_*), dropping the transient to_boxref() round-trip at both resolve_operand_operand call sites and the Operand::from_boxref lowering in the same_box check. Removes the last production from_boxref call in mod.rs. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 1, 2026
Flip `IndexVar.var_box: Option<BoxRef>` to `Option<Operand>` and bind the dormant-vectorizer producer sites, draining the last E5b (#175) residual. dependency.rs: - `var_box` field + `new_boxed` param BoxRef -> Operand; `get_or_create` passes the bound `arg_box.clone()` instead of `arg_box.to_boxref()`. - `get_operations` carries `var` as an Operand directly: `first_var` and the chained references bind a synthetic producer via `Operand::bound_from_opref` (to_opref-identical) instead of `BoxRef::from_opref`, dropping the three `Operand::from_boxref(&var_box(..))` wraps. Removes the BoxRef import (dependency.rs is now BoxRef-free). The constant arg stays `from_opref` (a ConstInt, sheds inline). guard.rs `rename_op`: `Operand::from_opref(replacement)` -> `bound_from_opref`. The renamer replacement is a producer position, so `from_opref` would panic on it; bind a synthetic producer carrying the same pos, matching the sibling guard-strengthening sites. Both paths are vectorizer-only (vec_all default false), so check.py cannot exercise them; correct by construction, mirroring the landed `bound_from_opref` siblings. Gate: dynasm-lib 1370 / cranelift-lib 1368 / ir 333 / check.py 167/167 both backends. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Flip `IndexVar.var_box: Option<BoxRef>` to `Option<Operand>` and bind the dormant-vectorizer producer sites, draining the last E5b (#175) residual. dependency.rs: - `var_box` field + `new_boxed` param BoxRef -> Operand; `get_or_create` passes the bound `arg_box.clone()` instead of `arg_box.to_boxref()`. - `get_operations` carries `var` as an Operand directly: `first_var` and the chained references bind a synthetic producer via `Operand::bound_from_opref` (to_opref-identical) instead of `BoxRef::from_opref`, dropping the three `Operand::from_boxref(&var_box(..))` wraps. Removes the BoxRef import (dependency.rs is now BoxRef-free). The constant arg stays `from_opref` (a ConstInt, sheds inline). guard.rs `rename_op`: `Operand::from_opref(replacement)` -> `bound_from_opref`. The renamer replacement is a producer position, so `from_opref` would panic on it; bind a synthetic producer carrying the same pos, matching the sibling guard-strengthening sites. Both paths are vectorizer-only (vec_all default false), so check.py cannot exercise them; correct by construction, mirroring the landed `bound_from_opref` siblings. Gate: dynasm-lib 1370 / cranelift-lib 1368 / ir 333 / check.py 167/167 both backends. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Flip `IndexVar.var_box: Option<BoxRef>` to `Option<Operand>` and bind the dormant-vectorizer producer sites, draining the last E5b (#175) residual. dependency.rs: - `var_box` field + `new_boxed` param BoxRef -> Operand; `get_or_create` passes the bound `arg_box.clone()` instead of `arg_box.to_boxref()`. - `get_operations` carries `var` as an Operand directly: `first_var` and the chained references bind a synthetic producer via `Operand::bound_from_opref` (to_opref-identical) instead of `BoxRef::from_opref`, dropping the three `Operand::from_boxref(&var_box(..))` wraps. Removes the BoxRef import (dependency.rs is now BoxRef-free). The constant arg stays `from_opref` (a ConstInt, sheds inline). guard.rs `rename_op`: `Operand::from_opref(replacement)` -> `bound_from_opref`. The renamer replacement is a producer position, so `from_opref` would panic on it; bind a synthetic producer carrying the same pos, matching the sibling guard-strengthening sites. Both paths are vectorizer-only (vec_all default false), so check.py cannot exercise them; correct by construction, mirroring the landed `bound_from_opref` siblings. Gate: dynasm-lib 1370 / cranelift-lib 1368 / ir 333 / check.py 167/167 both backends. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
…xVar.var_box to Operand (#321) * optimizeopt: ProducedShortOp res/same_as_source to Operand Flip ProducedShortOp.res and ProducedShortOp.same_as_source from BoxRef to Operand, along with the coupled PreambleOp (AbstractShortOp) same_as_source field. res is always a producer-bound or const operand (materialize_operand_at on the preview; res.bound_op()-rooted exported entries per #173), never position-only. Retype the record/add channel to carry Operand: record_imported_preamble_use, record_preamble_use, add_tracked_preamble_op, and ShortPreambleBuilder::new's short_boxes parameter. Replace materialize_box_at with materialize_operand_at at the five ProducedShortOp construction sites in mod.rs. Contain at the principled BoxRef boundaries via to_boxref (Rc::ptr_eq-stable on the bound producer): exported_infos (#158), info::PreambleOp / ImportedShortPureOp (#182/#183), the position-only PreambleOp.res channel, and label_args: Vec<BoxRef>. Delete the optimizer same_as_source position-remap block: an Operand live-tracks its producer's already-remapped Op.pos through the carried Rc<Op>, so there is no separate position Cell to rewrite; the position-only else-branch was dead since #173 roots the producer. Assisted-by: Claude * optimizeopt: ImportedShortAlias.same_as_source to Operand Flip ImportedShortAlias.same_as_source from BoxRef to Operand. Every construction site is producer-bound: production reads op.arg(0) (already an Operand op-arg) and the three test mints use rooted_resop_operand, so the field never carries a position-only box. This deletes a round-trip that the BoxRef field forced: used_imported_short_aliases stored op.arg(0).to_boxref(), and emit_alias_same_as_for_imports rebuilt the SameAs arg via Operand::from_boxref(&same_as_source). With the field typed Operand both conversions are dropped — op.arg(0) is stored directly and cloned into the emitted op. The assert on .to_opref() is unchanged (Operand carries it). Assisted-by: Claude * optimizeopt: bind IndexVar.var_box + rename_op to producers Flip `IndexVar.var_box: Option<BoxRef>` to `Option<Operand>` and bind the dormant-vectorizer producer sites, draining the last E5b (#175) residual. dependency.rs: - `var_box` field + `new_boxed` param BoxRef -> Operand; `get_or_create` passes the bound `arg_box.clone()` instead of `arg_box.to_boxref()`. - `get_operations` carries `var` as an Operand directly: `first_var` and the chained references bind a synthetic producer via `Operand::bound_from_opref` (to_opref-identical) instead of `BoxRef::from_opref`, dropping the three `Operand::from_boxref(&var_box(..))` wraps. Removes the BoxRef import (dependency.rs is now BoxRef-free). The constant arg stays `from_opref` (a ConstInt, sheds inline). guard.rs `rename_op`: `Operand::from_opref(replacement)` -> `bound_from_opref`. The renamer replacement is a producer position, so `from_opref` would panic on it; bind a synthetic producer carrying the same pos, matching the sibling guard-strengthening sites. Both paths are vectorizer-only (vec_all default false), so check.py cannot exercise them; correct by construction, mirroring the landed `bound_from_opref` siblings. Gate: dynasm-lib 1370 / cranelift-lib 1368 / ir 333 / check.py 167/167 both backends. Assisted-by: Claude * optimizeopt: flip util::args_eq/args_hash to Operand `args_eq` / `args_hash` / `hash_arg` take `&[Option<Operand>]` instead of `&[Option<BoxRef>]` (util.py:100-122 parity port). `Operand::same_box` carries the same const-by-value / producer-by-identity semantics, and `hash_arg` keeps the value special-case (`Operand::Const` hashes by pointer, so equal fresh consts must hash via `const_value` to preserve the args_eq/args_hash contract). Tests use `Operand::const_` / `bound_from_opref`. Drops the last BoxRef use in util.rs (import removed). These helpers have no production caller (the optimizer's CSE compares via `same_box` directly); kept as a faithful port. Lib-authoritative: dynasm-lib 1370 / cranelift-lib 1368 / ir 333. Assisted-by: Claude * optimizeopt: short-box map keys via materialize_operand_at `produce_arg` and `materialize_one` resolved the produced_short_boxes / boxes_in_production key with `let key = ctx.materialize_box_at(x); let okey = Operand::from_boxref(&key)`. Collapse both to `let okey = ctx.materialize_operand_at(x)` — byte-identical (`materialize_operand_at` is `from_boxref(&materialize_box_at(..))` with the same mutation side-effect), dropping two `from_boxref` bridges and the intermediate BoxRef locals. Gate: dynasm-lib 1370 / cranelift-lib 1368 / check.py 168/168 both backends. Assisted-by: Claude * optimizeopt: ShortPreamble position fields to OpRef Flip ShortPreamble.{inputargs, used_boxes, jump_args, phase1_inputargs} from Vec<BoxRef>/Option<Vec<BoxRef>> to flat Vec<OpRef>. These are cross-phase position-domain channels whose payload is a bare producer or const position. Re-home walk_const_ptr_refs_mut to forward inline OpRef::ConstPtr GcRefs directly over &mut slices instead of BoxRef::walk_const_ptr_refs; the old path round-tripped the same GcRef through a BoxKind::Const cell, so forwarding is unchanged. Drop the from_opref re-mints at producers and the to_opref extraction at consumers across shortpreamble.rs / unroll.rs / optimizer.rs. The ExtendedShortPreambleBuilder.used_boxes field stays BoxRef, so re-mint the positions at that boundary. Assisted-by: Claude * optimizeopt: ShortPreamble builder position fields to OpRef Flip the ShortPreambleBuilder-feeding position channels from BoxRef to flat OpRef: - ShortPreambleBuilder.known_constants (VecSet) and .used_boxes; ExtendedShortPreambleBuilder.used_boxes. - OptContext.exported_short_inputargs and ExportedState.short_inputargs. - initialize_imported_short_preamble_builder[_from_short_boxes] short_inputargs parameter. Re-home the const GC walk to inline OpRef::ConstPtr forwarding (visit_oprefs / visit_opref_set over &mut). Retire the temporary re-mint bridge C1 left: used_boxes assigns short_preamble.used_boxes directly; the produce loop pushes produced.preamble_op.pos.get(). Assisted-by: Claude * optimizeopt: PreambleOp.res to Operand PreambleOp.res is identity-bearing at the export boundary — its value feeds ProducedShortOp.res through Operand::from_boxref, which panics on a position-only box, so production res is always producer-bound or const. Carry it as Operand (matching ProducedShortOp.res) rather than a flat OpRef, preserving the producer Rc identity. - Export reads (build_short_preamble_struct, optimizer preview-to-export) become entry.res.clone() / produced.res.clone(). - Drop the optimizer res position-remap block: a bound/const operand live-tracks its producer's already-remapped op.pos, like same_as_source. - Construction: ctx sites use materialize_operand_at; the ctx-less collector builder uses Operand::bound_from_opref. - GC walk routes through Operand::walk_const_ptr_refs (Const cell get/visit/set), consistent with the sibling same_as_source field. - Tests use rooted_resop_operand / Operand::from_boxref. Assisted-by: Claude * optimizeopt: ExportedState position fields to OpRef Flip the three pure position-domain ExportedState fields end_args, renamed_inputargs, and runtime_boxes from Vec<BoxRef> to flat Vec<OpRef>. Their readers already resolve via to_opref (trace inputarg reconstruction in pyjitpl, high-water scan, remap), so storing OpRef drops the constructor from_opref re-mints and the reader round-trips. Re-home the const GC walk to inline OpRef::ConstPtr forwarding (visit_oprefs over &mut); high-water and export remap read the position directly (remap_opref). next_iteration_args and the exported_infos keys stay BoxRef — they carry cross-peel ptr_eq identity and convert together in a later slice. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Port the dependency-graph path/edge primitives that `analyse_index_calculations` needs (dependency.py): - Node::provides()/depends() — forward/backward edge lists (246-253). - Dependency::target_node()/origin_node()/is_failarg() (429-461). - DependencyGraph::iterate_paths() — index-based port of the Node.iterate_paths generator (303-352): worklist path enumeration with optional destination, direction, path_max_len cap, and blacklist. - Diamond-DAG unit test covering forward/backward/max-len enumeration. Narrow the vector.rs analyse_index_calculations deferral note: of the four cited blockers, imaginary_node (via add_imaginary_node), iterate_paths, and is_always_pure are now ported; only edge_to / remove_edge_to graph-mutation remain, plus #175 for mark_guard failargs. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Port analyse_index_calculations (vector.py:529-583): build the loop dependency graph, add an imaginary early-exit node, and reroute every valid guard's edges through it so guards fail early, returning the rewired graph as the reschedule basis. Add the graph-mutation primitives it needs to dependency.rs: - edge_to/remove_edge_to (index-based Node.edge_to/remove_edge_to), keeping the deps/users side-vectors the scheduler reads consistent. - Dependency::new regains the failarg argument (dependency.py:415-421); add_edge was dropping it on newly created edges. - schedule_operations excludes imaginary nodes from its output and indexes in_degree by node position rather than the Node.idx sentinel. mark_guard's failargs body stays deferred to #175. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 3, 2026
Port the dependency-graph path/edge primitives that `analyse_index_calculations` needs (dependency.py): - Node::provides()/depends() — forward/backward edge lists (246-253). - Dependency::target_node()/origin_node()/is_failarg() (429-461). - DependencyGraph::iterate_paths() — index-based port of the Node.iterate_paths generator (303-352): worklist path enumeration with optional destination, direction, path_max_len cap, and blacklist. - Diamond-DAG unit test covering forward/backward/max-len enumeration. Narrow the vector.rs analyse_index_calculations deferral note: of the four cited blockers, imaginary_node (via add_imaginary_node), iterate_paths, and is_always_pure are now ported; only edge_to / remove_edge_to graph-mutation remain, plus #175 for mark_guard failargs. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 3, 2026
Port analyse_index_calculations (vector.py:529-583): build the loop dependency graph, add an imaginary early-exit node, and reroute every valid guard's edges through it so guards fail early, returning the rewired graph as the reschedule basis. Add the graph-mutation primitives it needs to dependency.rs: - edge_to/remove_edge_to (index-based Node.edge_to/remove_edge_to), keeping the deps/users side-vectors the scheduler reads consistent. - Dependency::new regains the failarg argument (dependency.py:415-421); add_edge was dropping it on newly created edges. - schedule_operations excludes imaginary nodes from its output and indexes in_degree by node position rather than the Node.idx sentinel. mark_guard's failargs body stays deferred to #175. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 3, 2026
Port the dependency-graph path/edge primitives that `analyse_index_calculations` needs (dependency.py): - Node::provides()/depends() — forward/backward edge lists (246-253). - Dependency::target_node()/origin_node()/is_failarg() (429-461). - DependencyGraph::iterate_paths() — index-based port of the Node.iterate_paths generator (303-352): worklist path enumeration with optional destination, direction, path_max_len cap, and blacklist. - Diamond-DAG unit test covering forward/backward/max-len enumeration. Narrow the vector.rs analyse_index_calculations deferral note: of the four cited blockers, imaginary_node (via add_imaginary_node), iterate_paths, and is_always_pure are now ported; only edge_to / remove_edge_to graph-mutation remain, plus #175 for mark_guard failargs. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 3, 2026
Port analyse_index_calculations (vector.py:529-583): build the loop dependency graph, add an imaginary early-exit node, and reroute every valid guard's edges through it so guards fail early, returning the rewired graph as the reschedule basis. Add the graph-mutation primitives it needs to dependency.rs: - edge_to/remove_edge_to (index-based Node.edge_to/remove_edge_to), keeping the deps/users side-vectors the scheduler reads consistent. - Dependency::new regains the failarg argument (dependency.py:415-421); add_edge was dropping it on newly created edges. - schedule_operations excludes imaginary nodes from its output and indexes in_degree by node position rather than the Node.idx sentinel. mark_guard's failargs body stays deferred to #175. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 3, 2026
* Tighten translator symbol parity
* Tighten llmemory symbol parity
* Trim ruleopt and rrange symbol gaps
* Port rtype_builtin_range constructor lowering
Implement rtype_builtin_range / rtype_builtin_xrange in rrange.rs and
register both in rbuiltin.rs install_default_typers.
- nb_args 1/2/3 arms select vstart/vstop/vstep; const step of zero raises
TyperError.
- AbstractRangeRepr result: step != 0 -> gendirectcall ll_newrange,
step == 0 -> ll_newrangest. Non-range result (real list) returns a
deferred error citing the unported ll_newlist / ll_setitem_fast.
- build_ll_newrange_helper_graph: single block, malloc(RANGE.TO) flavor=gc
+ setfield start/stop.
- build_ll_newrangest_helper_graph: start block guards int_eq(step, 0)
raising ValueError, build block mallocs RANGEST + setfield start/stop/step.
- emit_gc_malloc / emit_void_setfield graph-op helpers.
Assisted-by: Claude
* Port DummyValueBuilder ll_dummy_value; sharpen GCRefRepr.ll_str note
Add RPythonTyper.cache_dummy_values and implement the lazy immortal
placeholder allocation for both dummy-value builders (rmodel.py:452-464,
rgcref.py:93-104):
- DummyValueBuilder::ll_dummy_value mallocs an immortal Struct/Array
placeholder (n=1 for _is_varsize) keyed by TYPE in cache_dummy_values;
the typer is threaded in at call time since the builder stores only
rtyper_id for identity.
- DummyValueBuilderGCRef::ll_dummy_value casts the base-instance dummy
(getinstancerepr(None) -> DummyValueBuilder over TYPE.TO) to GCREF and
memoises it under GCREF; return type changed from Constant to
LowLevelValue to match the base builder.
- The producer side (Repr.get_ll_dummyval_obj) stays deferred: its only
consumers are the unported dict/ordereddict ENTRIES.dummy_obj fields.
Sharpen the GCRefRepr.ll_str deferral note to cite the concrete blocker:
ll_str is carried upstream by VoidRepr/TupleRepr (gen_str_function)/
string reprs, none of which pyre exposes as an ll_str surface, so the
hasattr guard is vacuously false.
Assisted-by: Claude
* Classify rtyper and metainterp symbol gaps
* Tighten annlowlevel symbol parity
* Classify callparse holder symbols
* Classify llgroup carrier symbols
* Classify llinterp symbol gaps
* Restore jitprof profiler symbols
* Tighten jitprof symbol parity
Port the profiler class surface from jitprof.py:16-50:
- BaseProfiler (jitprof.py:16-17) marker struct.
- EmptyProfiler (jitprof.py:19-50) no-op profiler with the full
start/finish/tracing/backend/count/count_ops/get_counter/get_times
surface; initialized = true.
- Profiler type alias exposing the upstream public name for JitProfiler.
- BrokenProfilerData error marker.
Classify JitProfiler / JitProfilerSnapshot / ProfilerEventGuard as
intentional Rust-side extras in check-rpython-module-parity.py.
Assisted-by: Claude
* rtuple: drop redundant bare ll_tupleiter/ll_tuplenext stubs
The bare ll_tupleiter(&LowLevelType, Hlvalue) / ll_tuplenext(Hlvalue)
functions returned rtuple_deferred and had no production callers — a
value-returning function cannot emit the malloc/setfield operations these
helpers require. rtuple.py:399-411 is faithfully ported by the
graph-builder forms build_ll_tupleiter_helper_graph (malloc ITERPTR.TO +
setfield 'tuple') and build_ll_tuplenext_helper_graph (getfield/ptr_nonzero
/StopIteration/null-store/return item0), which are the ones newiter /
rtype_next actually gendirectcall. Move the upstream citations onto those
builders and remove the dead stubs plus their assert-deferred test.
Assisted-by: Claude
* Restore rtuple iterator helper symbols
* Align rtyper PBC naming parity
* Expose annlowlevel placeholder helpers
* Align rstr public symbol parity
* Align rclass public symbol parity
* Align rtyper public symbol parity
* Align ll_math public symbol parity
* dependency: fold ImaginaryNode into Node{op: Option}
Eliminate the separate-carrier deviation from dependency.py's Path/Node
model: Node.op becomes Option<Op> (None for imaginary nodes), and the
ImaginaryNode struct plus the PathNode enum are deleted. Path now stores
node indices uniformly (Vec<usize>), including imaginary nodes appended
to DependencyGraph.nodes via add_imaginary_node.
- Node::new_imaginary(label) builds an op=None node carrying a dotlabel
and a fake index; is_imaginary() == op.is_none(); getoperation()
returns Option<&Op>; op() unwraps for real-node call sites.
- Path::second/last/first/last_but_one return the actual node index at
each position (real or imaginary), matching dependency.py:56-72.
- set_schedule_priority sets priority on every segment including
imaginary ones (dependency.py:100-102), where the old split skipped
them; is_always_pure skips imaginary segments (dependency.py:84-86).
- Cascade node.op field reads to op()/getoperation() across dependency.rs
and vector.rs.
Assisted-by: Claude
* Add rffi make helper parity
* Classify lltypesystem symbol adaptations
* intbounds: emit _all_rules_fired rewrite-rule stats
Add per-rule fire counters to the autogen int-rule mixin, mirroring the
generated `_rule_names_<op>` / `_rule_fired_<op>` class attributes and
their `_all_rules_fired` registration (autogenintrules.py:18-22):
- `RULE_NAMES_*` / `RULE_FIRED_*` static tables plus `all_rules_fired()`
registry, and a `fire(counts, index)` helper for the in-line bumps.
- Bump the matching counter at each of the 136 `optimize_INT_*` rewrite
sites (`self._rule_fired_<op>[i] += 1`).
- Implement `print_rewrite_rule_statistics` (intbounds.py:862-870): dump
the counts in a `jit-intbounds-stats` debug section.
- Registry-shape and debug-dump smoke tests.
Assisted-by: Claude
* dependency: port iterate_paths + edge accessors
Port the dependency-graph path/edge primitives that
`analyse_index_calculations` needs (dependency.py):
- Node::provides()/depends() — forward/backward edge lists (246-253).
- Dependency::target_node()/origin_node()/is_failarg() (429-461).
- DependencyGraph::iterate_paths() — index-based port of the
Node.iterate_paths generator (303-352): worklist path enumeration with
optional destination, direction, path_max_len cap, and blacklist.
- Diamond-DAG unit test covering forward/backward/max-len enumeration.
Narrow the vector.rs analyse_index_calculations deferral note: of the
four cited blockers, imaginary_node (via add_imaginary_node),
iterate_paths, and is_always_pure are now ported; only edge_to /
remove_edge_to graph-mutation remain, plus #175 for mark_guard failargs.
Assisted-by: Claude
* vector: port analyse_index_calculations guard hoisting
Port analyse_index_calculations (vector.py:529-583): build the loop
dependency graph, add an imaginary early-exit node, and reroute every
valid guard's edges through it so guards fail early, returning the
rewired graph as the reschedule basis.
Add the graph-mutation primitives it needs to dependency.rs:
- edge_to/remove_edge_to (index-based Node.edge_to/remove_edge_to),
keeping the deps/users side-vectors the scheduler reads consistent.
- Dependency::new regains the failarg argument (dependency.py:415-421);
add_edge was dropping it on newly created edges.
- schedule_operations excludes imaginary nodes from its output and
indexes in_degree by node position rather than the Node.idx sentinel.
mark_guard's failargs body stays deferred to #175.
Assisted-by: Claude
* vector: complete mark_guard early-exit descr and failargs
mark_guard attaches a CompileLoopVersionDescr to GUARD_TRUE/GUARD_FALSE,
copying resume attributes from an existing descr, and sets the loop label
args as failargs via getarglist_operand.
- compile.rs: add make_compile_loop_version_descr / _typed / _with_payload.
- dependency.rs: set_edge_failarg propagates a failarg downgrade to both
the forward and back edge (dependency.py:457-458 dep.backward).
* rrange: build ll_range2list real-list arm
rtype_builtin_range list-result path lowers to ll_range2list through a
helper graph using ll_newlist and setitem.
- rlist: expose build_ll_newlist_helper_graph and item_lowleveltype.
* rordereddict: add dict iterator and lookup helper graphs
build_ll_dictiter_helper_graph, ll_dict_lookup, and ll_dictiter.
* Fix build warnings
This was referenced Aug 12, 2026
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…d deny through the warm state (#1364) * jit: name the fixture the inline-chain depth cap was measured on `FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude * majit: publish the preview short-preamble export as one PreviewShortState Replace the four OptContext fields `exported_short_boxes`, `exported_short_inputargs`, `exported_short_inputarg_refs` and `exported_short_args_state` with a single `Option<PreviewShortState>` holding the three vectors plus an `Option` args_state. `preamble_end_args` stays a separate `Option<Vec<OpRef>>`. optimizer.rs binds `create_short_inputargs`, `create_short_inputarg_refs`, the args-state tuple and the filtered short boxes to locals and assigns the struct once, after the majit_log dump. `force_box_for_end_of_preamble` and its `preamble_end_args` write are unmoved. unroll.rs::export_state_with_bounds reads the args-state through `preview_short_state.and_then(|p| p.args_state)`, and takes the short inputargs / inputarg refs / short boxes from one match on the `Option` instead of an `is_empty()` test on a defaulted vector. The `debug_assert_eq!` cross-checking `exported_short_inputarg_refs` against `exported_short_inputargs` is dropped; the two are now built and published together. The length check against the export-site `label_args + virtuals` recompute is kept. unroll.rs tests gain `publish_preview_short_state` and `mint_short_inputargs` helpers; the four fixtures that wrote the ctx fields directly now construct the struct. Assisted-by: Claude * docs: correct the jit.virtual_ref emit note and two fbw fixture headers gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude * docs: separate the MIFrame perform_call builds from the frame a vref is taken of The previous commit's gate-triage note called `pyjitpl.py:2445-2476` a wrong citation and said `mod.rs` held the last copy. Both overreach. The range spans `newframe` (:2455-2476), which does build a fresh frame per inlined call -- an `MIFrame`, the tracer's register frame. `pyre-jit-trace/src/helpers.rs` cites it for exactly that and is correct; stripping it there would remove a right citation. The same sentence also lives in `inline_call.rs`, so `mod.rs` was never the only copy. What is actually wrong at the two vref sites is conflating that `MIFrame` with the app-level frame `ExecutionContext.enter` takes `jit.virtual_ref` of. State that at both, and say in gate-triage which of the two claims the range supports. Also: the emit note said `virtualref_boxes` is populated for every seeded level, but `entered_ec` adds a non-null concrete frame and a non-null `execution_context` on top of seeding -- necessary, not sufficient. And the crn fixture header's inserted paragraph left "Its effects are idempotent" pointing at the `_declined` sibling instead of the drive. Assisted-by: Claude * majit: lower the int/float list capacity read as arraylen_gc `list.int_capacity` / `list.float_capacity` emitted `getfield_gc_r(<strategy>_items.block)` followed by a struct `getfield_gc_i(ItemsBlock.capacity)`. Upstream's capacity read is `len(l.items)` on `l.items: Ptr(GcArray(ITEM))` (rpython/rtyper/lltypesystem/rlist.py:251 in `_ll_list_resize_hint`, rlist.py:286 in `_ll_list_resize_ge`), which the rtyper lowers to `getarraysize` and `jtransform.py:808 rewrite_op_getarraysize` rewrites to `arraylen_gc`. `list.obj_capacity` already emitted `ArrayLen`. Both arms now emit `OpKind::ArrayLen` on the backing block, matching the object-strategy arm. The two unit tests are renamed and assert the new op shape. Assisted-by: Claude * majit: narrow the PreviewShortState alignment claim to the two vectors that share an index space The struct doc and the publication comment both said the three published vectors are "index-aligned by construction". Only two of them are: `short_inputargs` and `short_inputarg_refs` get one entry per `add_short_input_arg`. `short_boxes` is a different population -- the surviving produced short ops, after `short_boxes_exported`'s `filter_map` drops every one whose `canonical_result` is constant -- so its length is unrelated to the other two and `short_boxes[i]` pairs with nothing. What one publication site does buy is that a reader cannot see one vector from this evaluation of the preview beside another that was never written; both comments now say that instead. The unroll.rs test helper restates the refs/inputargs length invariant as a `debug_assert_eq!`. The production publisher gets it from `create_short_inputarg_refs`, which asserts internally; a fixture builds the two vectors by hand and had no check between a short refs vector and the failed `Weak` upgrade it causes past the peel boundary. Assisted-by: Claude * docs: correct eight comments that assert a capability the code already has Each of these states an absolute negative -- "never emits", "never calls it", "cannot be added and go unnamed" -- that its own call site contradicts. - `residual_call.rs`: `vrefs_after_residual_call` is called by the walker, under the `is_may_force` gate mirroring `pyjitpl.py:2007`. Its loops are empty because no `jit.virtual_ref` producers exist, which is a fact about the vref list, not about the call site. - `branch.rs` `decode_side_other_target`: the fused `goto_if_not_<cmp>` forms do reach the walk dispatch, minted by `majit-translate`'s jtransform as `ExitSwitch::Fused` for the LLBC-lowered graphs. What is true is narrower: the sole caller passes a `PyJitCode`, built per Python CodeObject by pyre's own codewriter, and `ExitSwitch::Tuple` -- the only path to a fused goto there -- has no producer outside `flatten.rs`'s own unit test. A fused form would be declined, not mis-decoded. - `inline_call.rs` / `fbw_state.rs`: `callee_body_contains_raise` caps a raising callee at TWO multiframe levels, not at the top inline level, and the cross-frame unwind bridge is built. The measurement beside `effective_multiframe_depth` is what bounds it: two levels green, a third taking `selfrec_tail_exception_unwind` from 937 to 7408 guard failures. - `state.rs` (two sites) / `helpers.rs`: `perform_call` (`pyjitpl.py:2445-2449`) is `newframe` + `setup_call` + `raise ChangeFrame`, and `newframe` (`:2455-2476`) builds an `MIFrame` and nothing else. Upstream has no recording-time app-level frame to hand out at that point; it gets one from tracing the interpreter's own frame construction, which pyre does not do. - `state.rs` / `trace.rs`: the pointer to `perform_call (trace_opcode.rs)` is dead -- no such function there. Callee sym state is set by `inline_call.rs`'s `setup_call` port. - `diag.rs` `SPEC_FOLD_ROWS`: the table cannot become a complete census by adding rows. It names a fold by its function, and two shapes have no name to give: a fold whose emit is inlined into a `match` arm has no function, and a registry-dispatched fold grows by one entry with no new call site. Also: `specialize.rs` drops line numbers from an in-repo file reference, and `tupleobject.rs` records that the `w_tuple_new` interception is the sole reason the `_ff` layout has no producer -- so restoring the upstream shape also makes the walker's `ff` specialisation arm live. Assisted-by: Claude * fbw: print all fourteen fbw_diag slots from both readers and namespace the escape/force keys The two readers of the same counter array printed disjoint index sets: the native reader (pyre/pyrex) printed {1, 6..13} and the wasm host (pyre-wasm-runner) printed {0..5, 11..13}, so slots 0 and 2..5 were bumped on the native backends and readable only through the wasm export, and slots 6..10 the other way round. One key per tally slot is now declared beside the counters as `pyre_jit_trace::trace::fbw_diag::LABELS` (length `RING_BASE`, so rustc rejects an unnamed slot), re-exported as `pyre_jit::FBW_DIAG_LABELS`, and joined against `get(i)` by both readers into a single `[jit-stats] fbw_diag` line carrying the same keys in the same order. The runner mirrors the array positionally, as it already does for `MC_DIAG_LABELS`, since it links no pyre crate. The MIDBODY_LATCH doc said the native corpus reaches neither leg "so these say whether the wasm target does"; it now says that a nonzero native value is itself the news, which is why both readers print it. On wasm the tally line moves out of the PYRE_WASM_JIT_STATS block — check.py never sets that variable — into the MAJIT_STATS block, the gate the native reader prints under. The `[fbw-census]` ring stays where it was. The four gated keys (fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, fbw_blackhole_adopted_single_frame, fbw_blackhole_adopted_multi_frame) keep their spelling and move from the counter line onto that fbw_diag line; the single `pyre_fbw_diag` lookup still feeds the missing-export refusal. The two `subset/total` fractions become named keys, the hazardous subset spelled `fbw_midbody_latch_new_unjournaled` and `fbw_escape_plain_fallback_unclean`. The five bare keys portal_only, published_callee_only, portal_and_published_callee, by_portal and by_callee_only are renamed fbw_escape_portal_only, fbw_escape_published_callee_only, fbw_escape_portal_and_published_callee, fbw_force_by_portal and fbw_force_by_callee_only: check.py's `_jit_stats_merged` folds every `[jit-stats]` line into one flat map, in which an un-namespaced key is a collision hazard. No committed .jitstats baseline carries any of the five under either spelling, so nothing is orphaned by the rename. They are left out of JITSTATS_SNAPSHOT_FIELDS, i.e. deliberately ungated, and check.py now records why: they are workload counts with no healthy value and no measured polarity (the reason `bridges_compiled` sits in neither regression list), and listing one would make every baseline that lacks it compare 0 -> N and fail until re-recorded. That re-record is a decision to take deliberately, with a polarity in hand. Checked with `cargo check -p pyrex`, `cargo check -p pyre-wasm-runner` and `cargo fmt --check`. No pyre binary was built or run, and no .jitstats snapshot was re-recorded. Assisted-by: Claude * fbw: make the decline census process-wide, as its own doc already claimed `FBW_DECLINE_CENSUS` was a `thread_local!` while the comment above it called it a "Per-process census". pyre installs `_thread` (`pyre-interpreter/src/importing.rs`), so Python threads are real OS threads and each traces on its own; the dump therefore reported only whichever thread happened to print it and silently dropped every decline the others took. Now a `static Mutex<BTreeMap>` behind a `census_map()` accessor that recovers from poisoning -- a map of counters has no invariant a panicking writer can leave broken, and a diagnostic that goes silent after an unrelated panic is worse than one that keeps counting. The lock costs nothing at this rate: the map is touched only on the cold decline path, never on the hot trace path. Pinned by `the_decline_census_counts_a_record_from_another_thread`, shown to fail on the `thread_local!` storage first: assertion `left == right` failed: a decline recorded off-thread never reached the census left: 0 right: 1 No gate exposure: `fbw_census` appears in no check.py field, no pyrex path and no committed baseline, so this changes a diagnostic only. Assisted-by: Claude * fbw: pin the wasm runner's fbw_diag label mirror against the slot constants `pyre-wasm-runner` links no pyre crate, so it restates `pyre_jit_trace::trace::fbw_diag::LABELS` as a positional array. rustc length-checks each side against its own constant (`RING_BASE` / `FBW_SLOTS`), but neither compiler sees the spellings, so a rename drifts silently and every tally from the divergence onward is printed under the wrong key -- and check.py folds every `[jit-stats]` line into one flat map, so a wrong name is compared against the wrong baseline rather than reported as missing. Four checks, following `majit-metainterp/tests/mc_diag_mirror.rs`: the parser is validated against the compiler-enforced count before being used to diagnose drift, the two declared counts must agree, and the two arrays must agree entry by entry. A positive control injects both drift shapes into the real runner source in memory -- a rename, caught by the entry comparison, and a dropped last slot, which leaves every surviving entry correctly named and so can only be caught by the length check. Perturbing the real text rather than a fixture is what makes the control cover the anchors. A fifth check closes what a two-array diff structurally cannot see: `LABELS` shifting against the slot CONSTANTS moves both arrays together, renaming every tally on both backends at once. Each label is bound to its own constant, and the bound slots are required to be exactly `0..RING_BASE` so a new slot cannot go unbound. The bindings are written out rather than derived from the constant names because two of them break the mechanical reading: `ESCAPE_FORCE_BY_PORTAL` is `fbw_force_by_portal`, not `fbw_escape_force_by_portal`. Assisted-by: Claude * docs: name the third fold shape SPEC_FOLD_ROWS structurally cannot hold The table names a fold by its function. Two shapes with no name to give were already recorded; a sweep of `vable_ops.rs` found a third. An ELISION fold recognises a shape and emits nothing, so a census keyed on "what IR did this fold emit instead" has nothing to key on. Three arms are this, all guarded by `fbw_strict_fold_frame_reg`: a store to the current inline level's own unseeded portal frame is a virtual-field write, folded away with no SETFIELD_GC recorded. Their recognisers (`fbw_strict_fold_frame_reg`, `folded_store_is_observable_local`) are predicates -- they cannot emit, because the eliding is the arm. This is distinct from the functionless-replace shape already listed: `bool_box_truth_lookup`'s arm has no function but does write a result. Assisted-by: Claude * majit-translate: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` citations in codewriter/ and annotator/ comments with the symbol that owns the cited code, verified by opening each target. Upstream `.py` line citations are untouched. String-literal occurrences (assertion and panic messages) are untouched. Also drop the internal tracking labels `Z2.5 Path C`, `Phase I3` and `F2 followup`, the filename-less `(line ~3273)` pointer, and replace insns.rs's "documented at the const-table site above" with the fact that byte 18 now houses `BC_GOTO_IF_NOT`. Four citations are left as-is because their target no longer exists: `build_flow.rs:215` (file deleted with the syn-AST front-end) in call.rs and codewriter.rs, and `parse.rs:314-318` (parse.rs shrank from ~2000 to 98 lines) twice in call.rs. Assisted-by: Claude * pyre-jit: re-anchor comment references from line numbers to symbols Replace every `file.rs:NNN` citation in pyre/pyre-jit comments with the symbol that lives at the cited location, or with the bare filename where the surrounding text already names the symbol. Upstream `.py` citations (rpython/pypy/lib-python) are untouched. Also replace directional cross-references ("see below", "see comment above", "see the deferral below") with the named symbol they point at, drop the internal "Slice α-2" and "Phase L2" markers, and drop stale self-file line refs ("line 1891", "line 2120-2122", "at line 1495"). Comment-only; no code, string literal or test data changed. Assisted-by: Claude * optimizeopt: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude * pyre-jit-trace: re-anchor pyre-side comment refs to symbol names Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude * docs: correct three comments refuted by their own call sites Each claimed a capability was missing; each is contradicted by the code it sits next to. `descr.rs`'s tag block says the Field tag is load-bearing for a synthetic `FieldIndexDescr` that unpacks offset/size/type/signed out of the index bits. That descriptor and its helpers were deleted — `majit-ir`'s descr module records the removal — and `VirtualizableFieldState.fields` is keyed by `FieldDescr::index_in_parent()` now (`info.py:203-206`). Nothing decodes the tag; what it still buys is disjoint index ranges so two descr kinds cannot collide on one `HeapCache` key. The `ptr_eq/rr>i` opcode-table row says the `b1 is b2` fast path is omitted, "same rationale as int comparisons". Both handlers implement it: `binop_ref_to_int_record` answers an identical operand pair out of `fastpath_same_boxes` without recording, and so does `binop_int_record`. The `raise` arm says resume-data capture is omitted, pointing at the `goto_if_not/iL` arm, which carries no such comment. The guard this arm emits calls `walker_capture_snapshot_for_last_guard(ctx, op.pc)` twelve lines below, which is `generate_guard`'s `resumepc=orgpc`. Assisted-by: Claude * descr: mark PyCode.co_firstlineno immutable, per _immutable_fields_ `pycode.py:95-106` lists `co_firstlineno` in `_immutable_fields_`; the PyCode descr group marked every field mutable because its spec builder hard-coded the flag. Give the builder the flag as a parameter and set it from the upstream list: only `co_firstlineno` changes. `co_name` and `hidden_applevel` are absent from that list and stay mutable — `w_name` is realized lazily by `w_code_name_obj` and does go null -> non-null after construction — and `code_ptr` is the raw body pointer with no upstream slot. The slot really is write-once: `box_code_constant_with_firstlineno` writes it onto an object `box_code_constant` has just boxed out of a fresh `Box`, so no caching lets a reader see it first, and `code.replace` reads it and builds a new code object rather than writing this one. No behaviour change is expected or observed. The only trace-side reader of a field descr's `is_immutable` is the replay-cleanliness rule in `fbw_state.rs`, which fires on a `setfield_gc` into a freshly allocated object, and traced Python never constructs a PyCode. `check.py --no-build --backend dynasm`: 441/441, no jitstats delta. Assisted-by: Claude * majit-metainterp: replace pyre-side line refs in comments with symbol names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude * majit-translate: replace pyre-side comment line refs with symbol names Strip `file.rs:NNN` / `file.rs:NNN-MMM` line numbers from comments in majit/majit-translate/src (excluding codewriter/ and annotator/) and majit/majit-translate/tests, keeping the symbol name the comment already cited or naming the enclosing item where the citation had none. Upstream `.py` citations are untouched. Also: - qualify ambiguous bare `model.rs` references to `flowspace/model.rs` or `annotator/model.rs` where the named symbol resolves there - drop self-referential filename parentheticals in rclass.rs, rpbc.rs, rtyper.rs, mir.rs, flowspace_adapter.rs, cutover.rs and rbuiltin.rs - point flowspace_adapter.rs's exc_from_raise cross-reference at the "TODO: `Constant` SSA carrier shape" section that exists in that module preamble - remove the "slice A" / "Slice C" tracking labels from flowspace_adapter.rs and llinterp.rs Comment-only: no code, string literal or test data changed. Assisted-by: Claude * pyre-interpreter: replace pyre-side line-number comment refs with symbol names Rewrite `<file>.rs:NNN` citations in comments under pyre/pyre-interpreter to name the symbol at the cited location instead of a line number, or drop the line number where the symbol was already named. Citations against the pinned rustpython-compiler-core and rustpython-sre_engine snapshots (`oparg.rs`, `bytecode/instruction.rs`, `string.rs`, `engine.rs`) are left as they are. Also replace directional cross-references ("see above", "see below", "the note above", "as noted above") with the fact or the owning symbol, and drop the "B1" tracking prefix from a jit_fnaddr comment. Comment-only; no code, string literal or test data changed. Assisted-by: Claude * Re-anchor pyre-side comment references to symbols Replace `<file>.rs:NNN` line citations in comments across majit-backend-{dynasm,cranelift,wasm}, majit-macros, majit-ir, majit-gc and pyre-object with the file name alone, or with the symbol that the cited line's enclosing item defines where the cited location still matches the comment's claim. Upstream `rpython/`, `pypy/`, `lib-python/` and `lib_pypy/` `.py:NNN` citations are unchanged, as is the `compiler.rs:12884` reference inside the `bridge_cache_addrs` expect string. Symbols named where verified: `bh_call_r` / `bh_call_f` / `bh_call_v` default trait impls, `gc_rewriter`, `do_compile`, `emit_guard_exit`, `cranelift_realloc_frame`, `dynasm_typeid_subclass_range`, `generate_state_fields_jit_state`, `generate_trace_fn`, `handle_new`, `gen_malloc_nursery`, `gen_write_barrier`, `handle_write_barrier_setfield`, `do_collect_nursery`, `rescan_major_nonstack_roots_and_drain`, `register_active_hooks`, `CompiledLoopToken`, `next_op_can_accept_cc`, `AbstractVirtualPtrInfo`. Replace three cross-reference pointers with the fact they pointed at: the `write_float_at_mem` "see read sibling above", the `reg_write_audit` "see the module doc", and the wasm `stamp_and_publish_label_targets` "the comment below". Drop the internal tracking ids `S-11`, `Phase E.3+`, `Slice 80-G.7` and `Pre-A.2.3` from the comments they appeared in. Assisted-by: Claude * fbw: record the measured mechanism behind the loop-bearing blackhole handoff decline The `walk_abort_adopted` deny-list arm for `LoopBearingCalleeInlineUnsupported` carried a comment saying its second blocker was open and that dropping the arm produces wrong code, without naming a cause. Measured it on both failing fixtures and replaced that paragraph with the mechanism. `bhimpl_jit_merge_point` treats a frame that has a `nextblackholeinterp` as the recursive portal level: it takes `bhimpl_recursive_call_*`, parks the result in `tmpreg_*` and raises `LeaveFrame`. A multi-frame image stacks the callee above its caller, and this decline reports that the callee bears a loop, so the callee reaches its own loop-header merge point before any `*_return` and the caller below it receives `tmpreg_*` as the callee's return value. Both fixtures leave at that opcode with `ret_type=Ref`: `inline_subwalk_user_iterator` on `[run@1054, step@260]`, and `list_append_write_barrier_gc` on `[big_live_len_regrow@1191, churn@162]`. Comment only; no behaviour change. Assisted-by: Claude * fbw: name the caller-image refusal, the vref bracket's size, and the two qmut decline causes Three diagnostics, all gated on fbw_debug_abort_enabled, no behaviour change. capture_inline_parent_blackhole answered None from its three liveness-pass early returns without printing anything, so the downstream "parent.blackhole None (capture missing)" could not say which bank, which color, or whether the walk's shadow was merely too short. Added report_caller_image_decline and wired it into the int, ref and float sites. Each site changed from `...get(color).copied()?` to an explicit `let Some(..) = got else`, which separates an out-of-range color from one whose shadow holds a different concrete kind; the `?` conflated them. The vref bracket's two halves iterate virtualref_boxes and nothing reported its length, so its size was only ever restated from which call sites populate it. Print the pair count above vrefs_before_residual_call. The qmut flush leg printed one decline message for both WalkEndResume variants that can be unprovable. Split it: RewindUnproven means no opcode-entry sample was taken, a still-unprovable Rewind means the opcode had already applied an effect. Assisted-by: Claude * fbw: correct two docs that call the vref bracket's loops empty Both said the bracket's loops are empty because no jit.virtual_ref producers exist. A producer does exist and runs: walker_ec_enter takes a vref of every seeded callee frame through TraceCtx::opimpl_virtual_ref, paired with opimpl_virtual_ref_finish when the frame leaves. Measured with the [vref-bracket] report over 431 synth + 93 parity fixtures: 5487 bracket entries, 686 of them (12.5%) with at least one pair, 66 of the 316 emitting fixtures reaching a nonzero count, maximum 7 pairs. Assisted-by: Claude * docs: name the produced-view source PreviewShortState::short_boxes `produced_short_boxes_from_exported_boxes`'s header still cited `ctx.exported_short_boxes`, a field removed when the preview export was collapsed into `PreviewShortState`. Point it at the surviving field. Assisted-by: Claude * descr: state PyCode field purity per field in its spec test `pycode_field_descrs_share_parent_and_preserve_specs` asserted `!descr.is_always_pure()` for every PyCode field. Marking `co_firstlineno` immutable made that field answer true — `is_always_pure()` returns the `immutable` flag — so the test has been failing since that change; its commit verified with `check.py` only, which does not run crate unit tests. Add the expected purity to the per-field tuple and compare it, so each field states its own answer and a move in either direction fails. Also re-point one comment in optimizer.rs at `exported_short_boxes`, the name the local kept after the preview-export collapse. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: answer a caller image's unstamped ref color instead of refusing the image `capture_inline_parent_blackhole`'s liveness pass demanded a `ConcreteValue::Ref` for every ref color live at the resume pc and returned `None` for the whole image otherwise. The innermost-frame fill `build_single_frame_miframe` (residual_call.rs) runs the same pass and answers the two ways that demand fails: * a live color whose register holds no box is skipped — a `-live-` set is the union over the paths INTO its coordinate, so a color can be live there and undefined on the path walked, and `_copy_data_from_miframe` (`blackhole.py:1711-1730`) likewise leaves a `None` box unset; * a color whose shadow is `ConcreteValue::Null`, the walker's untracked sentinel that `write_ref_reg` stamps for every recorded-but-unobserved result, is recovered through `TraceCtx::recover_ref_value`. Port both. The image is still refused when neither applies. Measured with a report added at the refusal site, over 431 synth + 93 parity fixtures (dynasm, darwin): 21 refusals across 10 fixtures, all bank `r` with a `Null` shadow, partitioning as 11 no-box and 10 recoverable and 0 neither. After the change the corpus reports none. The report itself stays, extended with the box and its recoverability, and the header's decline count is corrected: it cited the downstream `[s2-build-decline]` symbol, which prints only when a multi-frame build was attempted and so undercounted tenfold. `check.py --no-build`: dynasm 441/441, cranelift 441/441, wasm 434/434, no jitstats delta. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: route the hazardous-inline deny through disable_noninlinable_function `fbw_abort_nested_unjournaled_residual` names the callee an abort is attributable to and denies it, but the deny wrote only the walker-local `FBW_HAZARDOUS_INLINE_DENY` thread-local, so the callee's JitCell never carried `JC_DONT_TRACE_HERE` and no warm-state reader saw it. It now also calls `disable_noninlinable_function` on `make_green_key(callee_code, 0)`, the function-entry key `inline_call.rs` already uses for that callee — the same answer `pyjitpl.py:2818-2828` gives for the callee `find_biggest_function` names. The consuming half — `warmstate.py:485-496`, where a `JC_DONT_TRACE_HERE` cell that has never seen a procedure token retraces at once instead of waiting out the counter — is already carried by `WarmEnterState::maybe_compile_decision`. Measured (dynasm): the three fbw witness fixtures now mint the denied callee's cell, cells 3 -> 4 on each. `get_stats` counts the `BaseJitCellState` enum rather than the flag, so its `dont_trace_here` reads 1 only on `wasm_ca_trampoline_decline`; on the two `foriter_exempt_*` fixtures the new cell has already moved on to tracing (tracing 0 -> 1) and the state no longer names the deny its flag still records. `list_append_write_barrier_gc` gains a compiled loop (loops_compiled 12 -> 13), re-recorded on all three backends; the wasm baseline is from a measured wasm run. Assisted-by: Claude * fbw: name which hazard clause denied the nested-residual inline `fbw_inline_callee_hazardous` fires on three clauses and returned only the callee's code key, so the `PYRE_LB_SITE=1` `[lb-arm]` line could say `hazard=true` and nothing more. It now returns the clause name alongside the key and the report prints it: `hazard=repeat`, `hazard=for-iter`, `hazard=self-recursive`, or `hazard=false`. The clauses are not equally tight. `repeat` and `self-recursive` name the frame that is actually recursing; `for-iter` is `code_has_for_iter`, which fires on any code object whose bytecode contains a `FOR_ITER` anywhere, whether or not an iterator is in flight at the decline point. Census over 441 synth + 83 parity fixtures: for-iter 8 fires / 6 fixtures, self-recursive 2 / 2, repeat 2 / 2. Only two of the six `for-iter` fixtures are the witnesses that clause documents. Same denial set as before — the three clauses are checked in the same order and return the same key. check.py --no-build --backend dynasm: 441/441. cargo test --release -p pyre-jit-trace: rc=0. Assisted-by: Claude * warmstate: drop the DontTraceHere state and count the denial off its flag `JC_DONT_TRACE_HERE` had two representations: the flag, and a `BaseJitCellState` variant. Every real decision already read the flag — `can_inline_callable`, `counter_tick_checked`, `should_start_dont_trace_here_trace`, and `should_remove_jitcell` — while `is_compiled` and `is_tracing` read the token and `JC_TRACING`. The state variant reached only `get_stats`, and the two answers disagreed: `disable_noninlinable_function` set the state only when `JC_TRACING` was clear, so a cell denied on its way into a trace carried the flag but never took the state, and the census counted zero denials on every fixture that reaches the fbw hazard arm. The flag is now the only representation. `get_stats` counts it directly and independently of the lifecycle state, which is what makes a denied-then-tracing cell visible; `is_dont_trace_here` reads it; the two `state == DontTraceHere` tests in `counter_would_fire` and `counter_tick` were unreachable behind the flag test on the line above and are gone. warmstate.py has no such state either: `JC_DONT_TRACE_HERE` is orthogonal to the lifecycle — a denied cell still traces, compiles, and is invalidated, and `warmstate.py:485-496` retraces it once its procedure token dies. So the abort paths now leave `BaseJitCellState::NotHot` and set the flag alone, which also collapses `abort_tracing`'s three branches into the single condition `abort_tracing_for_key` already used. cargo test --release -p majit-metainterp --features dynasm: rc=0. Assisted-by: Claude * fbw: record that narrowing the for-iter hazard clause is wrong code `fbw_inline_callee_hazardous`'s `for-iter` clause is deliberately loose — it fires on any callee whose bytecode contains a `FOR_ITER`, in flight or not — and the census this branch added shows it carrying 8 of the 32 declines across 441 synth + 83 parity fixtures for 2 witnesses. Narrowing it to "a consume already ran in this frame" is measurable and wrong. `FBW_FORITER_INFLIGHT` answers that question without the per-frame Python pc `InlineFrame` lacks, since its `Jit` entries carry the `jitcode_index` each consume ran in, and it does cut the clause to 3 fires with both witnesses still declining. But `foriter_exempt_shared_generator` then produces wrong output on all three backends, `inline_subwalk_user_iterator` regresses (loops_aborted 1 -> 5, fbw_rolled_back_with_effects 0 -> 5, loops_compiled 3 -> 2) and `list_append_write_barrier_gc` loses its compiled loop again (13 -> 12). The witness still declined, just at pc 533 instead of 261: inlining the residual is what carries the walk to the consume, so a test conditioned on the consume having happened is always one step late. The clause has to stay forward-looking, and a real narrowing needs FOR_ITER reachability from the frame's current position — which is where the missing per-frame pc actually bites. Assisted-by: Claude
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
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.
Summary
Self-review
Prompt & Model
Model:
Prompt:
Answer
Summary by CodeRabbit
install-charon.pyandextract-llbc.pyfor installing Charon and generating LLBC artifacts..ullbcoutputs are missing/empty; updated formatting to run with the pinned Rust toolchain andcargo fmt -- --check.