Repository navigation
jit: resolve struct layouts for the target being built, not the extraction host - #707
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThe PR adds target-specific LLBC layout sidecars throughout extraction, caching, discovery, and runtime validation. It also switches layout calculations to target word size and removes wasm append-fold gating while narrowing metainterpreter snapshot and integer-bound handling. ChangesCross-target LLBC layouts
JIT list append dispatch
Metainterpreter corrections
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant LLBCExtractor
participant Charon
participant BuildScript
participant JITRuntime
CLI->>LLBCExtractor: configure layout targets
LLBCExtractor->>Charon: extract target LLBC
Charon-->>LLBCExtractor: full target artifact
LLBCExtractor->>LLBCExtractor: write layout sidecar
BuildScript->>JITRuntime: load required LLBC and sidecars
JITRuntime-->>BuildScript: validate build-time field offsets
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 91e898a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc0cdff0ab
ℹ️ 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".
| // carry the extraction host's. Without them every descr field past the | ||
| // first pointer names the wrong bytes on a target whose pointers are a | ||
| // different width, which corrupts reads instead of failing. | ||
| for name in llbc_layout_sidecars() { |
There was a problem hiding this comment.
When cross-compiling to any target other than wasm32-unknown-unknown (for example aarch64-unknown-linux-gnu from an x86 host), this preflight now requires pyre-object.<target>.layouts.ullbc and pyre-interpreter.<target>.layouts.ullbc, but the bootstrap command printed below still runs scripts/extract-llbc.py with the driver default of only LAYOUT_TARGETS = ("wasm32-unknown-unknown",). That command can never create the target-specific files being reported missing, so these cross builds stay stuck unless the user already knows to set LLBC_LAYOUT_TARGETS; please pass/advertise the active target or make the extractor default match the preflight requirement.
Useful? React with 👍 / 👎.
| for sidecar in llbc_layout_sidecars() { | ||
| println!("cargo::rerun-if-changed={repo_root}/build/llbc/{sidecar}"); |
There was a problem hiding this comment.
These sidecars are now build inputs for cross-target descriptor generation, but they are only registered with Cargo rerun tracking; codegen_cache_key() still hashes just the three host .ullbc files in hash_llbc_inputs(). If a sidecar is regenerated with the same Rust sources and target (for example after fixing the sidecar reducer/extraction flags), the build script reruns and then restores the old all_descrs.bin under the unchanged cache key, so the corrected target offsets are ignored until the cache is manually cleared.
Useful? React with 👍 / 👎.
| if std::mem::size_of::<usize>() != 8 { | ||
| return None; |
There was a problem hiding this comment.
On 32-bit targets this knowingly routes around the orthodox list-append sub-walk because the comment says it still produces wrong output and that “the remaining defect is in the fold”. That leaves a root-cause JIT bug hidden behind a target-width gate and drops the optimization for every wasm32 build instead of fixing the fold or disabling the specific invalid transformation with a regression test, which conflicts with the repository debugging discipline against root-cause workarounds.
AGENTS.md reference: AGENTS.md:L205-L207
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28fb8e6e0
ℹ️ 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".
| "--targets", | ||
| target, |
There was a problem hiding this comment.
This new target-specific extraction is still gated by the stamp computed above from source_fingerprint(), whose metadata walk is filtered to the extraction host, not this target. When a wasm32-only dependency/source changes, the existing *.wasm32-unknown-unknown.layouts.ullbc passes the sidecars existence check and the unchanged host fingerprint makes the crate skip, so cross builds keep stale target field offsets until --force or a host-visible file changes. The sidecar stamp/cache needs to include the target metadata/source closure, or otherwise hash the sidecar-producing inputs before trusting the cached sidecar.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
|
Two more commits: the 32-bit list-append fold now actually folds. The remaining 32-bit defect was an array base, not a resume. The measurement that pinned it —
check.py after: dynasm 246/246, cranelift 246/246, wasm 245/245 — and 245/245 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 828a097557
ℹ️ 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".
| // on `SimpleInteriorFieldDescr.array_descr`. | ||
| let item_size = compute_struct_size(self, &elem_name); | ||
| let base_size = self.array_header_size; | ||
| let base_size = self.array_items_base(self.array_header_size, Some(&elem_name), item_size); |
There was a problem hiding this comment.
On wasm32, this makes the struct-array descriptor report an element-aligned base (e.g. 8 when a virtual array element needs 8-byte alignment), but guard-resume materialization still allocates and fills VArrayStructInfo through pyre_object::allocate_array_struct / setinteriorfield, whose GcTypedArray item base is GC_TYPED_ARRAY_ITEMS_OFFSET = size_of::<usize>() (4 on wasm32). In that case resume writes the reconstructed struct fields starting at byte 4 while any later getinteriorfield_gc using this descriptor reads from byte 8, so guard-failure resume can corrupt virtual arrays of structs; the materializer needs to use the descriptor base size or the descriptor must match the runtime block layout.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
828a097 to
847afd3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 847afd30cd
ℹ️ 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".
| "translated": { | ||
| "crate_name": field("crate_name"), | ||
| "type_decls": field("type_decls"), | ||
| "fun_decls": [], | ||
| }, |
There was a problem hiding this comment.
When a cross-target sidecar keeps only type_decls and replaces the rest of translated with an empty fun_decls, it can drop the HashConsedValue bodies and trait_impls that tyref_to_ast_string needs to decode type-decl field types (front/mir.rs:12398-12403). Because these sidecars are prepended and struct_fields are first-writer-wins in the LLBC merge (front/mir.rs:210-225), any field type that now renders as ??unresolved_dedup#... or an unresolved associated type prevents the complete host LLBC from repairing the metadata, so cross-target descriptor generation can classify such fields as pointer-sized defaults even though only the exact offsets were meant to come from the sidecar. Preserve the metadata needed to decode type_decls, or merge only exact_layouts from sidecars.
Useful? React with 👍 / 👎.
847afd3 to
b2f724c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f724c537
ℹ️ 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".
| for name in llbc_layout_sidecars() { | ||
| if !llbc_dir.join(&name).exists() { | ||
| missing.push(name); | ||
| } |
There was a problem hiding this comment.
When this loop adds target sidecars to missing during a cross-target build, the recovery text below still advertises PYRE_MIR_FRONTEND_LLBC with only the three host .ullbc files. That env var is treated as a complete input set and preflight_llbc_or_fail returns before checking sidecars, so following the hint on wasm32/aarch64 builds bypasses the new target layouts and regenerates descriptors from host offsets. Please require or print the sidecar paths in the override for cross-target builds.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/llbc_extract.py (1)
352-386: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFingerprint doesn't cover
layout_cargo_args/layout_target_rustflags, so those changes are cache-invisible.
stamp_forbakes inlayout_targets=crate_layout_targets(eng, spec)but not the actual per-target build inputs:spec.layout_cargo_argsandeng.layout_target_rustflags.source_fingerprintonly hashes crate source files. So changingLAYOUT_TARGET_RUSTFLAGS(e.g. a differentgetrandom_backend) or a crate'slayout_cargo_argsleaves the stamp unchanged, and the skip-check at Line 520-527 will happily reuse a stale sidecar that was built with the old flags — silently serving outdated target layouts, which is exactly the failure mode this PR set out to eliminate.🐛 Proposed fix: fold the layout build inputs into the stamp
def stamp_for( eng: Engine, *, crate: str, platform_key: str, charon_stamp: str, cargo_features: str, flags: list[str], charon_flags: list[str], layout_targets: list[str], + layout_flags_by_target: dict[str, list[str]], + layout_target_rustflags: str, ) -> str: return "\n".join( [ f"crate={crate}", f"platform={platform_key}", f"charon={charon_stamp}", f"features={cargo_features}", f"flags={' '.join(flags)}", f"charon_flags={' '.join(charon_flags)}", f"layout_targets={' '.join(layout_targets)}", + f"layout_flags={layout_flags_by_target}", + f"layout_rustflags={layout_target_rustflags}", f"source={source_fingerprint(eng, [crate], cargo_features)}", ] )Also applies to: 514-519
🤖 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/llbc_extract.py` around lines 352 - 386, Update stamp_for and its callers so the cache stamp includes the effective per-target layout build inputs: spec.layout_cargo_args and eng.layout_target_rustflags, alongside layout_targets. Ensure the values are serialized deterministically for every target, so changes to either configuration invalidate the skip-check and rebuild stale layout sidecars.majit/majit-translate/src/codewriter/call.rs (1)
1014-1067: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThree heuristic layout paths cap scalar/struct alignment at
target_word_size()instead of a fixed 8, reintroducing on wasm32 the exact misalignment class this PR fixes for arrays.element_align'sscalar_align(Lines 1432-1453) already gets this right —size.clamp(1, 8).next_power_of_two(), independent oftarget_word_size()— because wasm32's own LLVM data layout givesi64/f648-byte alignment despite 4-byte pointers. The three sites below instead dosize.min(crate::layout::target_word_size()), which on wasm32 caps ani64/f64field's alignment at 4 bytes.
majit/majit-translate/src/codewriter/call.rs#L1014-L1067: inStructLayout::from_type_strings, change the typeptr/pad branch (Line 1019), the regular per-field align (Line 1031), andmax_align(Lines 1059-1060) to cap at a fixed 8, nottarget_word_size().majit/majit-translate/src/codewriter/call.rs#L6666-L6698: inall_interiorfielddescrs's Path-2 heuristic, apply the same fixed-8 cap at Line 6677 (a plaini64/f64field does not hit the earlier nested-struct bailout at Lines 6644-6648, so it reaches this buggy cap).majit/majit-translate/src/codewriter/call.rs#L6774-L6824: incompute_struct_size, cap both the per-field align (Line 6800) andmax_align(Lines 6810-6811) at a fixed 8 instead oftarget_word_size().Based on path instructions ("The generated JIT must preserve the interpreter's semantics ... Fix divergence as a generation defect"), this is exactly the kind of target-layout divergence that should be fixed at the source rather than left as a latent heuristic-path gap.
🐛 Suggested shared-cap fix (illustrative, apply the same clamp at each site)
- let align = field_size.min(crate::layout::target_word_size()); + let align = field_size.clamp(1, 8).next_power_of_two();🤖 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/majit-translate/src/codewriter/call.rs` around lines 1014 - 1067, Replace target_word_size()-based alignment caps with a fixed 8-byte cap to preserve i64/f64 alignment on wasm32. In majit/majit-translate/src/codewriter/call.rs:1014-1067, update StructLayout::from_type_strings’ typeptr/pad alignment, per-field alignment, and max_align; in 6666-6698, update the all_interiorfielddescrs Path-2 heuristic; and in 6774-6824, update compute_struct_size’s per-field alignment and max_align. Keep the existing size and layout behavior unchanged apart from using the fixed 8-byte maximum.Source: Path instructions
🤖 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 `@majit/majit-metainterp/src/optimizeopt/intbounds.rs`:
- Around line 1718-1725: Update have_postprocess_op to register only the integer
field-load variants handled by propagate_postprocess, removing the
GetfieldRawR/F, GetfieldGcR/F, and GetinteriorfieldGcR/F variants. Keep the
dispatcher capability check aligned with the narrowed match.
In `@majit/majit-translate/src/lib.rs`:
- Around line 297-316: Update the cross-target sidecar handling in the
path-building logic around MANDATORY so each expected sidecar is explicitly
checked and a warning is emitted when it is missing, matching the existing
pyre-jit.ullbc diagnostic behavior. Continue adding existing sidecars to paths,
but make missing cross-target layout artefacts visible before proceeding.
In `@pyre/pyre-jit-trace/build.rs`:
- Around line 596-614: Centralize the target/host check and sidecar naming logic
currently duplicated by llbc_layout_sidecars() and
majit_translate::auto_discover_workspace_llbc_paths. Add a public helper in
majit-translate that returns the applicable pyre-object and pyre-interpreter
layout sidecar names, then update both callers to use it while preserving the
empty result for native or missing-target builds.
In `@scripts/llbc_extract.py`:
- Around line 388-427: Harden the nested field() helper in write_layout_sidecar
against incorrect substring matches and malformed JSON decoding. Validate each
decoded field against its expected shape, especially type_decls, and catch
json.JSONDecodeError to raise the existing clean SystemExit message instead of a
traceback; ensure invalid matches cannot silently populate the sidecar with the
wrong value.
---
Outside diff comments:
In `@majit/majit-translate/src/codewriter/call.rs`:
- Around line 1014-1067: Replace target_word_size()-based alignment caps with a
fixed 8-byte cap to preserve i64/f64 alignment on wasm32. In
majit/majit-translate/src/codewriter/call.rs:1014-1067, update
StructLayout::from_type_strings’ typeptr/pad alignment, per-field alignment, and
max_align; in 6666-6698, update the all_interiorfielddescrs Path-2 heuristic;
and in 6774-6824, update compute_struct_size’s per-field alignment and
max_align. Keep the existing size and layout behavior unchanged apart from using
the fixed 8-byte maximum.
In `@scripts/llbc_extract.py`:
- Around line 352-386: Update stamp_for and its callers so the cache stamp
includes the effective per-target layout build inputs: spec.layout_cargo_args
and eng.layout_target_rustflags, alongside layout_targets. Ensure the values are
serialized deterministically for every target, so changes to either
configuration invalidate the skip-check and rebuild stale layout sidecars.
🪄 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: 27e3363e-df23-4ba6-a1a3-816f70c9decb
📒 Files selected for processing (12)
.github/workflows/pyre-ci.ymlmajit/majit-metainterp/src/history.rsmajit/majit-metainterp/src/optimizeopt/intbounds.rsmajit/majit-translate/src/codewriter/call.rsmajit/majit-translate/src/layout.rsmajit/majit-translate/src/lib.rspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/scripts/extract-llbc.pyscripts/llbc_extract.py
💤 Files with no reviewable changes (2)
- majit/majit-metainterp/src/history.rs
- pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
| /// Layout-sidecar artefact names this build consumes, empty for a native | ||
| /// build. | ||
| /// | ||
| /// `scripts/extract-llbc.py` re-extracts each crate for every cross target | ||
| /// and reduces the result to its `type_decls`; `majit-translate`'s | ||
| /// `auto_discover_workspace_llbc_paths` merges them ahead of the host | ||
| /// artefacts so their Charon-resolved field offsets win. | ||
| fn llbc_layout_sidecars() -> Vec<String> { | ||
| let target = std::env::var("TARGET").unwrap_or_default(); | ||
| if target.is_empty() || target == std::env::var("HOST").unwrap_or_default() { | ||
| return Vec::new(); | ||
| } | ||
| // `pyre-jit` is absent: it has no sidecar (see the extraction driver's | ||
| // spec for why), so only the object model gets cross-target layouts. | ||
| ["pyre-object", "pyre-interpreter"] | ||
| .iter() | ||
| .map(|crate_name| format!("{crate_name}.{target}.layouts.ullbc")) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
llbc_layout_sidecars() duplicates the same target/host + naming logic already inlined in majit-translate/src/lib.rs::auto_discover_workspace_llbc_paths.
Since this build script already depends on majit_translate directly (it constructs majit_translate::jitcode::JitCode, majit_translate::CompiledJitDriver, etc. elsewhere), the sidecar-name derivation ("{crate}.{target}.layouts.ullbc", target-vs-host check, the ["pyre-object", "pyre-interpreter"] list) could be a single pub fn in majit-translate that both this file and lib.rs's auto_discover_workspace_llbc_paths call, instead of two hand-synced copies that can silently drift if the naming convention or crate list changes in one but not the other.
🤖 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/pyre-jit-trace/build.rs` around lines 596 - 614, Centralize the
target/host check and sidecar naming logic currently duplicated by
llbc_layout_sidecars() and majit_translate::auto_discover_workspace_llbc_paths.
Add a public helper in majit-translate that returns the applicable pyre-object
and pyre-interpreter layout sidecar names, then update both callers to use it
while preserving the empty result for native or missing-target builds.
…t does not match the target The build-time global descr pool (`jitcode_runtime::all_descrs`, serialised by `pyre-jit-trace/build.rs`) carries field offsets computed with an 8-byte pointer: `majit-translate`'s `get_type_flag` sizes every Ref/usize field at 8 and `StructLayout::from_type_strings` aligns to the build-script process's `size_of::<usize>()`, and the Charon LLBC `field_offsets` come from the same x86_64 extraction. On wasm32 the real structs pack pointers at 4 — `PyObject.w_class` sits at 4, not 8; `PyType.instantiate` at 24, not 32 — so a concrete read through one of those descrs returns garbage. The orthodox `w_list_append` inline sub-walk is the only production consumer of that pool (`RawDescrPool::Global`). Under wasm32 its `is_plain_int1` subclass test folded on garbage (`ptr_eq(16, 119)` instead of the pinned `w_class`), so the walk descended the dead `switch_to_object_strategy` leg, executed that leg's residuals concretely against the live list, and then declined at the leg's un-lowered `ListStrategy::Object` ctor. The resulting abort left an unrollbackable body effect, so `fbw_foriter_inflight_take` refused delivery and dropped the in-flight FOR_ITER item: the loop ran one iteration short. `synth/delete_negative_open_slice_hot` printed the `range(N-1)` result on wasm. Add `jitcode_runtime::build_descr_layout_matches_target()` and consult it in `orthodox_list_append_body_and_sym`, which declines before any IR or side effect; the generic residual append handles the call. Constant-true on 64-bit, so the native backends are unchanged. dynasm 241/241, cranelift 241/241, wasm 240/240.
…ction host The build-time descr pool `pyre-jit-trace/build.rs` serialises is read back by the target binary, but every offset in it came from a single host extraction of Charon LLBC and from a hardcoded 8-byte pointer in the layout model. On a 32-bit target both are wrong: `PyObject.w_class` sits at 4, not 8, and `PyType.instantiate` at 24, not 32. - scripts/llbc_extract.py, pyre/scripts/extract-llbc.py: after the host extraction, re-run charon with `--targets <triple>` per configured layout target and reduce the result to its `type_decls`, written as `<crate>.<target>.layouts.ullbc`. `ensure_charon_std` adds the target to charon's toolchain first. The sidecar list joins the extraction stamp. pyre configures `wasm32-unknown-unknown`; `corpus` and `pyre-jit` opt out. - majit-translate: `auto_discover_workspace_llbc_paths` prepends the sidecars when TARGET differs from HOST, so their `exact_layouts` win the first-writer-wins merge. `layout::target_word_size()` reads `CARGO_CFG_TARGET_POINTER_WIDTH` and replaces `size_of::<usize>()` throughout `codewriter::call`; `usize`/`isize` split off `u64`/`i64` accordingly. - pyre-jit-trace/build.rs: preflight and rerun-if-changed cover the sidecars. - jitcode_runtime: `build_descr_layout_matches_target()` is replaced by `build_time_field_offset(owner, name)`, plus a test pinning the pool's `PyObject.w_class` and `PyType.instantiate` offsets to `offset_of!`. - majit-metainterp/optimizeopt/intbounds: `propagate_postprocess` narrowed the sub-word field-read arm to the `_I` opcodes. The `_R`/`_F` variants have no postprocess, and a 4-byte pointer field satisfied the sub-word test, asking for an integer bound on a Ref box. - pyre-ci.yml: cache the sidecars alongside the host artefacts. The orthodox list-append sub-walk still declines on 32-bit targets; the reason recorded at the gate is now the wrong output six bench/synth list programs produce under the wasm backend with the fold enabled, not the layouts.
`orthodox_list_append_body_and_sym` no longer returns `None` when `size_of::<usize>() != 8`. With the descr pool built from the target's own layouts, the arm wasm mis-resumes is the unboxed one, and `wasm_unboxed_append_fold_declined` already declines that at recognition; the word-size test only additionally blocked the Object-storage arm, which resumes correctly. check.py: dynasm 246/246, cranelift 246/246, wasm 245/245.
… the wasm unboxed-append decline
`arraydescrof_concrete` and the interior-field array descr both placed the
first item at `len_offset + array_header_size`, i.e. immediately after the
length word. The blocks these descrs address are `#[repr(C)] { capacity:
usize, items: [T; 0] }`, where the items begin at the length word rounded up
to `T`'s alignment. With an 8-byte word the two always coincide; with a
4-byte one an `i64` / `f64` item sits at 8 while the descr said 4, so the
Integer/Float list storage was addressed a half-word early and every
interpreter read returned two packed 32-bit halves — `[j & 3 for j in
range(100000)]` on wasm32 yielded elements like `1 << 32`.
- call: add `array_items_base` / `element_align` and use them at both descr
sites. A scalar or pointer element aligns to its own width capped at 8; a
struct element aligns to its widest registered field, falling back to the
word. Unit test pinned.
- jitcode_dispatch: delete `wasm_unboxed_append_fold_declined` and its three
uses. It declined the Integer/Float `w_list_append` fold on wasm32 for the
symptoms this addressing produced.
check.py: dynasm 246/246, cranelift 246/246, wasm 245/245 with the fold
folding on every backend.
… struct-array base matches GcTypedArray - front/mir + lib: a layout sidecar's body-stripped `type_decls` cannot decode a field type first hash-consed in a dropped function body, so its `struct_fields` carry unresolved types. Because field types are target-independent, take them from the host and only the target offsets from the sidecar: append the sidecar after the host artefacts (was: prepend) and merge `exact_layouts` last-writer-wins (was: first-writer). The sidecar's per-type-string tables now lose to the host's. - codewriter/call: revert the interior-field struct-array base to the bare `array_header_size`. Those descrs address `GcTypedArray`, a flat block whose items sit at `GC_TYPED_ARRAY_ITEMS_OFFSET = WORD` regardless of element alignment; element-aligning the base diverged from the block (and from the guard-resume materialiser) on a 32-bit target. The list int/float storage (`TypedItemsBlock`, genuinely element-aligned) keeps the aligned base. - optimizeopt/intbounds: drop the `_R`/`_F` field-load opcodes from `have_postprocess_op`; `propagate_postprocess` handles only the `_I` variants, so those dispatched to a no-op. - pyre-jit-trace/build.rs: hash the layout sidecars into the codegen cache key (a regenerated sidecar rekeys instead of restoring stale descrs); advertise `LLBC_LAYOUT_TARGETS=<target>` and the sidecar paths in the cross-target recovery messages. - majit-translate/layout: `is_cross_target` + `layout_sidecar_filename` centralise the sidecar naming shared by build.rs and `auto_discover_workspace_llbc_paths`, which also now warns on a missing sidecar. - llbc_extract: `write_layout_sidecar`'s field scan validates the decoded type and turns a `JSONDecodeError` into the file's clean `SystemExit`. check.py: dynasm 248/248, cranelift 248/248, wasm 247/247.
b2f724c to
91e898a
Compare
|
Worked through every review comment. Commit AppliedSidecars drop Struct-array base must match
Cross-target recovery hints can't produce/advertise sidecars (codex Sidecars not in the codegen cache key (codex Missing sidecar silently skipped (coderabbit Duplicated sidecar naming (coderabbit
Not applied"Fix the fold instead of gating it" (codex Fingerprint target-only inputs before skipping sidecars (codex check.py: dynasm 248/248, cranelift 248/248, wasm 247/247. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e898a2dc
ℹ️ 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".
| /// the alignment every length-prefixed block already satisfies. | ||
| fn element_align(&self, elem: Option<&str>, item_size: usize) -> usize { | ||
| let word = crate::layout::target_word_size(); | ||
| let scalar_align = |size: usize| size.clamp(1, 8).next_power_of_two(); |
There was a problem hiding this comment.
When cross-compiling to 32-bit targets whose ABI only 4-aligns 64-bit scalars (for example i686, whose Rust target data layout reports f64:32:64), this derives align = 8 purely from the item width. The runtime block is TypedItemsBlock with items at offset_of!(TypedItemsBlock, items) (pyre/pyre-object/src/object_array.rs:558-565), so on those targets the first i64/f64 item is at byte 4, not 8; the generated ArrayDescr will make unboxed int/float list reads and writes start one word late. Please use the target ABI/Charon-resolved alignment instead of next_power_of_two(item_size).
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
The build-time descr pool that
pyre-jit-trace/build.rsserialises is read backby the target binary, but every offset in it came from a single host extraction
of Charon LLBC plus a hardcoded 8-byte pointer in the layout model. On a 32-bit
target both are wrong —
PyObject.w_classsits at 4, not 8;PyType.instantiateat 24, not 32 — so the orthodox
w_list_appendsub-walk, the one productionconsumer that resolves
doperands through the pool, read the wrong bytes onwasm32. #697 papered over this by gating the fold on a runtime layout check.
Three layers, all fixed here:
cross target: charon re-runs with
--targets <triple>and the result isreduced to its
type_decls, written as<crate>.<target>.layouts.ullbc(1.1 MB / 6.9 MB vs the 46 MB / 383 MB host artefacts).
majit-translateprepends them when
TARGET != HOST, so theirexact_layoutswin thefirst-writer-wins merge. Reader code is untouched. Passing
--targetswithseveral triples in one pass was tried first and rejected: it aggregates
bodies too, which breaks portal resolution.
layout::target_word_size()reads
CARGO_CFG_TARGET_POINTER_WIDTHand replacessize_of::<usize>()throughout
codewriter::call;usize/isizesplit offu64/i64. On a64-bit target the word is still 8, so native codegen is bit-identical.
intbounds::propagate_postprocessgrouped
Getfield*{I,R,F}into one arm gated onfield_size < 8. Only the_Iopcodes have a postprocess, and a 4-byte pointer field satisfies thesub-word test — so with correct layouts wasm hit
setintbound: expected 'i'-typed operand, got Ref. Narrowed to_I.build_descr_layout_matches_target()is replaced bybuild_time_field_offset(owner, name)plus a test pinning the pool's offsets tooffset_of!for the structs the sub-walk reads.pyre-jitgets no sidecar: with--targetset, cargo stops applying RUSTFLAGSto host units, and
pyre-jit-trace's build script drags inrustpython-host_env,which uses
cfg_select!— still unstable on Charon's pinned nightly, and the-Zhost-configthat would reach host units panics cargo. The 588 types thisleaves out are all compiler-internal; every runtime type reached through a descr
comes from
pyre-objectorpyre-interpreter, which do get sidecars.Verification
check.py: dynasm 244/244, cranelift 244/244, wasm 243/243cargo test -p majit-metainterp --lib: 1401 passedcargo fmt --all -- --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests