Descend pos_inner for +x, and key a virtual's class word off the layout - #1561
Open
youknowone wants to merge 8 commits into
Open
Descend pos_inner for +x, and key a virtual's class word off the layout#1561youknowone wants to merge 8 commits into
youknowone wants to merge 8 commits into
Conversation
…ion sites that erased it `ResidualSlot` / `ResidualRet` and the `p*` / `cp*` / `pa*` / `cpa*` publishers already reject a helper the residual-call ABI cannot describe, and `push_abi_unsound_*` names the ones it cannot. Three sites reached `push_raw_fnaddr` without either, so nothing read their signatures. - `PYRE_TYPE_OBJECT_FNADDRS` carries each accessor as `fn() -> PyObjectRef`, so publish through `p0` rather than casting to `*const ()` first. - The twelve bigint `int_*` arithmetic and comparison helpers are `extern "C" fn(i64, i64) -> JitBigIntResult` and `-> i64`; publish them through `cp2`, which their five neighbours in the same block already used. - The six arity-to-address accessors in `runtime_ops` select a helper by a runtime count and must erase the signature to return it. `word_fn_addr!` ascribes the fn item to an explicit `extern "C" fn(i64, ..) -> i64` before the erasure, so the check runs where the type still exists. Writing the wrong machine-argument count is the same build error as a helper drifting off the word ABI, which is how the check is exercised. `push_alias_pair` becomes `push_word_accessor_alias_pair` and records that its address arrives already checked. `push_raw_fnaddr` now lists the three kinds of caller that may reach it. The published set is unchanged: 1326 entries over 971 distinct addresses, with the same paths and the same address grouping before and after. Assisted-by: Claude
…swering void `getkind` raises for a type it cannot classify. Both places pyre projects a `ConcreteType` onto a kind character answered `'v'` for `Unknown`, which is the kind of an absent argument and of a function returning nothing: the start-block inputarg projection would describe an argument the callee reads as not passed, and `graph_result_kind` would tell callers to discard a result the callee produces. Neither is a build error, so the mismatch would arrive as a wrong register count at trace-time call. `Unknown` is also the state a `Variable` carries before the rtyper sets its `concretetype` cell, so what reaches these two sites unresolved is a front-end gap rather than a type outside the kind space. Measured over the built image before changing anything, by recording every arm both sites take: 2948 graphs, arguments 3883 `r` / 498 `i` / 7 `f` / 0 `v`, results 4096 `r` / 906 `i` / 552 `v` / 30 `f`, and the `Unknown` arm taken zero times at either site. `result_kind_tests` keeps the refusal armed from a graph still carrying the pre-rtyper `returnvar`, with a resolved-type control beside it. Assisted-by: Claude
…p's descr `optimize_setfield_gc` recorded `PyObject.w_class` on a virtual at the `index_in_parent` its own descr carried. The tracer stores through a shared header descr whose index is 0, which on `W_BaseException` happens to name the class word and on `W_LongObject` names the payload at offset 16. Read the slot from the layout instead, through `SizeDescr::class_word_index_in_parent`, which is defined against `all_fielddescrs` -- the list `force_box` reads the vector back through. A layout that keeps its class word out of that list answers `None` and takes the previous arm: removable when the allocation already writes that class, otherwise emitted after forcing. `structinfo_setfield` declines a header word on a virtual. Its key comes from `heap::OptHeap::field_slot_index`'s band, which names no position, and `force_box` resolves that key to nothing. `VirtualInfo::fields` states the invariant as "every key is a slot of `all_fielddescrs()`" and names the two keys that are not, and `force_box` prints the failing key and the list it was looked up in. Assisted-by: Claude
`bh_size_spec_from_callcontrol` decided the explicit `[tag@0 | payload@8]`
shell with `ends_with("result::Result::Ok")` and two siblings, which also
accepts a user enum declared at `mycrate::option::Option::Some`.
Match the owner segment by segment as a suffix of a `core`- or `std`-rooted
path, so every spelling `front/mir` produces still resolves and a lookalike
rooted elsewhere does not. A test enumerates the twelve accepted spellings and
six rejected ones.
Assisted-by: Claude
…e_value `pos_inner` is `pos` past its `__pos__` override probe, whose `needs_numeric_unaryop_dispatch` is `dont_look_inside` and is the second operation the body executes. `inline(never)` keeps it a graph of its own; without it rustc folds the body into `unary_positive_value`. `jit_unary_positive_value` joins its invert and negative siblings in `jit_trace_fnaddrs`. Assisted-by: Claude
The method carried a default returning `type_error("unary_positive not
implemented")`. A trait method with a default body is what the graph walker
resolves to, so `PyFrame`'s implementation -- and with it `pos` -- was never
reached; `+x` compiles to `CALL_INTRINSIC_1`, which makes this method the whole
route. Declaring it required leaves the single implementation as the
resolution.
`PyFrame::unary_positive` now routes through `opcode_ops::unary_positive_value`,
matching its `list_to_tuple` sibling.
Assisted-by: Claude
`try_walker_orthodox_unary_positive` walks the `pos_inner` body behind a `GUARD_CLASS`, the shape the invert and negative descents take. It sits ahead of the `unary_positive_int` fold, so that fold's `consulted` count reads whether the descent took the site; the fold stays as the arm for a build whose `pos_inner` jitcode is absent or unlowered. `pos_inner_jitcode` resolves the body by graph key and caches it per thread. Assisted-by: Claude
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
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 |
![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: 3be4e0206a
ℹ️ 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".
| && matches!( | ||
| segments[segments.len() - 1].as_str(), | ||
| "malloc_typed" | "malloc_typed_managed" | ||
| "malloc_typed" | "malloc_typed_managed" | "malloc_typed_stable" |
There was a problem hiding this comment.
Do not treat malloc_typed_stable as equivalent to the movable allocators here. For any traced allocate_stable/malloc_typed_stable constructor matching this generic aggregate pattern—including the newly traced W_LongObject path—the pass replaces the call with NewWithVtable, while the GC rewriter uses the nursery unless the independently built SizeDescr is marked non_moving; this predicate does not propagate that requirement. Such objects are explicitly allocated stable because raw object pointers or promoted constants must survive collections, so lowering them to movable allocations can leave stale pointers after relocation. Either retain the residual stable allocator or carry the non-moving property into the generated allocation descriptor.
AGENTS.md reference: AGENTS.md:L29-L32
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3be4e02). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is here
Five commits: a
w_classslot fix in the optimizer, an assembler predicate follow-up, and the+xdescent that replaces the hand-writtenunary_positive_intfold as the primary arm.majit: key a virtual's class-word store off the layout, not off the op's descroptimize_setfield_gcrecordedPyObject.w_classon a virtual at theindex_in_parentits own descr carried. The tracer stores through a shared header descr whose index is0, which onW_BaseExceptionhappens to name the class word and onW_LongObjectnames the payload at offset 16.Reading the slot from the layout instead —
SizeDescr::class_word_index_in_parent, defined againstall_fielddescrs, the listforce_boxreads the vector back through — is right for both. A layout that keeps its class word out of that list answersNoneand takes the previous arm.b67e8abdb8dis red without this:pyre/check.py --backend dynasmreported 78 failed / 433 passed with 46 fixtures panicking inforce_boxon the key0x80000008. With the fix it is 510 passed, and the two remaining are pre-existing (below).structinfo_setfieldalso declines a header word on a virtual: its key comes fromheap::OptHeap::field_slot_index's band, which names no position at all.majit: match the explicit-shell variant owners against the whole pathReview follow-up from #1518.
ends_with("result::Result::Ok")also accepts a user enum atmycrate::option::Option::Some, whichfront::option_ctornever gives a shell to. Matching segment by segment against acore/stdroot keeps every spellingfront/mirproduces and rejects the lookalike; a test enumerates the twelve accepted and six rejected spellings.The
+xdescentOpcodeStepExecutor::unary_positivecarried a default body. A trait method with a default body is what the graph walker resolves to, soPyFrame's implementation — and with itpos— was never reached, and+xcompiles toCALL_INTRINSIC_1, which makes this method the whole route. Declaring it required opened the chain:unary_positiveunary_positive_valuepospos_innerpos_innerispospast its__pos__override probe, which isdont_look_insideand is the second operation the body executes. The descent then fires 2/2 and 1/1 and takes the site entirely (unary_positive_int consulted=0). The fold stays behind it for a build whosepos_innerjitcode is absent or unlowered.Gate
pyre/check.py --backend dynasm: 2 failed, 510 passed.cargo fmt --all -- --check,scripts/check-majit-boundary.pyandpyre/check.py --check-headersare clean;majit-metainterp(1704) andmajit-translate(3326) unit tests pass.Both remaining failures are pre-existing and not this branch's. Built at the merge base
903151ee679with none of this branch's commits, the same host reproduces them exactly:903151ee679exception_escape_hot_callee_tb_node_onceguard_failures 808 -> 1015exception_traceback_frame_linenobridges_compiled 6 -> 4,guard_failures 825 -> 624903151ee679is the commit that recorded both snapshots, so the committed numbers come from a different host. Re-recording them here would write this machine's numbers into a shared snapshot, so both are left alone.— commented by Claude