Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions majit/majit-gc/src/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,24 @@ pub struct TypeRegistry {

impl TypeRegistry {
/// Maximum number of types the backing `layout_table` can hold
/// without reallocating. Sized generously above the dozen-or-so
/// types pyre currently registers; bumps require recompiling
/// majit-gc.
/// without reallocating; bumps require recompiling majit-gc.
///
/// RPython's `type_info_group` is bounded by the half-word width
/// of the inline `GROUP_MEMBER_OFFSET` (translator/c/src/llgroup.h)
/// — 64KB on 32-bit and 4GB on 64-bit. majit's bound is set by
/// the maximum number of distinct GC types we expect to register,
/// not by an addressing limit.
pub const MAX_TYPES: usize = 1024;
/// not by an addressing limit: `Header` gives the type id the whole
/// low 32 bits (`header::TYPE_ID_BITS`), so this constant costs
/// pre-allocated capacity and nothing else.
///
/// It was 1024, sized for "the dozen-or-so types pyre registers".
/// Lowering the named-struct transparent constructors to `New`
/// (`jtransform`) turns each one into a registered GC type and put
/// the count past that: `PYRE_GC_TYPE_COUNT=1` reports 1046 frozen.
/// Read that number before choosing the next value rather than
/// inferring one from the `register` assert, which names the cap it
/// blew and not the total it needed.
pub const MAX_TYPES: usize = 4096;
}

/// Custom trace function type.
Expand Down Expand Up @@ -975,6 +983,15 @@ impl TypeRegistry {
return;
}
self.can_add_new_types = false;
// The `register` assert says to bump `MAX_TYPES` but not to what;
// this is the number to size it against.
if std::env::var_os("PYRE_GC_TYPE_COUNT").is_some() {
eprintln!(
"[gc-type-count] frozen={} max={}",
self.entries.len(),
Self::MAX_TYPES
);
}
self.assign_inheritance_ids();
// Refresh layout_table rows for object types whose
// subclassrange_{min,max} just changed.
Expand Down
19 changes: 13 additions & 6 deletions majit/majit-translate/src/codewriter/assembler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3596,7 +3596,7 @@ impl crate::translator::rtyper::lltypesystem::llmemory::OffsetLayout for NoStruc
}
}

fn bh_size_spec_from_callcontrol(
pub(crate) fn bh_size_spec_from_callcontrol(
cc: &CallControl,
owner: &str,
) -> Option<crate::jitcode::BhSizeSpec> {
Expand Down Expand Up @@ -3630,10 +3630,17 @@ fn bh_size_spec_from_callcontrol(
// `index_in_parent`; without the base tag in this list both the tag and
// payload are slot 0 and the payload replaces the tag despite living at
// byte offset 8.
// `Option::Some` is on this list for the same reason its two `Result`
// siblings are: `front/mir.rs` gives it the same explicit shell, so the
// blackhole must reconstruct the same inherited tag slot or the payload
// takes slot 0 here while living at byte 8 there. `None` carries no
// payload row, so it needs no reconstruction.
let result_variant = layout_owner.ends_with("result::Result::Ok")
|| layout_owner.ends_with("result::Result::Err")
|| layout_owner.ends_with("option::Option::Some")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Anchor the Option variant check to the core module

When a non-core enum is declared under a path ending in option::Option::Some (for example, mycrate::option::Option::Some), this suffix check classifies it as the synthetic core Option shell even though front::mir only assigns that shell to core::option::Option. Since bh_size_spec_from_callcontrol is also used by field descriptor lookup, the user enum's payload is consequently described at offset 8 with an injected discriminant instead of using its registered native layout, so reads and writes can access the wrong bytes. Use the same fully anchored core-path predicate as option_variant_ctor_tag rather than an ends_with match.

Useful? React with 👍 / 👎.

|| layout_owner == "Result::Ok"
|| layout_owner == "Result::Err";
|| layout_owner == "Result::Err"
|| layout_owner == "Option::Some";
// The bare Charon variant can contain only the inherited enum tag while
// the annotator keeps the payload row on the concrete instantiation
// (`Result<PyObjectRef, PyError>::Ok`). RPython has one concrete low-level
Expand Down Expand Up @@ -3759,10 +3766,10 @@ fn bh_all_field_specs_for_struct(
specs
}

/// Build the variant-owned portion of the explicit `Result<T, PyError>`
/// shell. The front end deliberately represents this as one inherited tag
/// word followed by one-word payload fields (`front/mir.rs`'s
/// `synthetic_result_shell`), rather than as Rust's native enum layout. The
/// Build the variant-owned portion of the explicit `Result<T, PyError>` /
/// `Option<T>` shell. The front end deliberately represents these as one
/// inherited tag word followed by one-word payload fields (`front/mir.rs`'s
/// `explicit_sum_shell`), rather than as Rust's native enum layout. The
/// shared nominal layout registry cannot describe each generic payload at
/// once, so read the concrete instantiation's rows directly and place them
/// after the inherited tag, matching RPython's concrete low-level STRUCT.
Expand Down
4 changes: 3 additions & 1 deletion majit/majit-translate/src/codewriter/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,9 @@ fn call_target_repr(target: &crate::model::CallTarget) -> String {
CallTarget::FunctionPath { segments } => {
format!("$<* function '{}'>", segments.join("."))
}
CallTarget::SyntheticTransparentCtor { name, owner_path } => {
CallTarget::SyntheticTransparentCtor {
name, owner_path, ..
} => {
if owner_path.is_empty() {
format!("$<* synthetic-transparent-ctor '{name}'>")
} else {
Expand Down
67 changes: 64 additions & 3 deletions majit/majit-translate/src/codewriter/jtransform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3702,7 +3702,9 @@ impl<'a> Transformer<'a> {
// Keep the owner-path gate: the universal MIR tuple aggregate has no
// Rust nominal owner, whereas a user ADT whose leaf happens to start
// with `Tuple` is a different allocation policy.
if let CallTarget::SyntheticTransparentCtor { name, owner_path } = target
if let CallTarget::SyntheticTransparentCtor {
name, owner_path, ..
} = target
&& owner_path.is_empty()
&& majit_ir::descr::is_shaped_tuple_name(name)
&& args.is_empty()
Expand All @@ -3725,7 +3727,9 @@ impl<'a> Transformer<'a> {
// Bare `Array` is deliberately excluded: only `Array<T;N>` carries a
// complete low-level owner identity. The owner-path gate keeps nominal
// user types out of this builtin aggregate policy.
if let CallTarget::SyntheticTransparentCtor { name, owner_path } = target
if let CallTarget::SyntheticTransparentCtor {
name, owner_path, ..
} = target
&& owner_path.is_empty()
&& majit_ir::descr::is_shaped_array_name(name)
&& args.is_empty()
Expand All @@ -3742,6 +3746,60 @@ impl<'a> Transformer<'a> {
},
}]);
}
// A niladic named-struct ctor is an allocation, exactly like the tuple
// and array forms above: `front::mir` emits the constructor with empty
// args and the operands follow it as `FieldWrite`s in program order,
// which is `rtyper`'s `malloc(GcStruct)` + one `setfield` per field.
// Left as a call it has no registered function address and puts its
// stable hash in JitCode as though it were executable code, which is
// what stops a descent that reaches one.
//
// `is_struct` is the whole gate, and it is why this cannot be spelled
// by inspecting the name. A sum-type variant carries a tag Charon does
// not spell as a MIR operand, so allocating one here without stamping
// it would leave the discriminant reading whatever the payload wrote;
// only `Result` and `Option`, whose variant order the language fixes,
// can have one stamped back (below). `front::mir` sets the flag only
// where it resolved a `TypeDeclKind::Struct`, and every other
// construction path leaves it false.
//
// `is_struct` alone is not enough to allocate. Charon models a
// closure environment as an Adt whose decl is a `Struct`, so the flag
// is true for one, and a closure has no registered layout at all. Ask
// the assembler's own question instead -- `bh_size_spec_from_callcontrol`
// is exactly what `OpKind::New` calls at emit time, and it panics there
// rather than declining, so anything it cannot answer must not become a
// `New` here.
//
// A layout answer is still not enough: the spec must also list fields.
// A field-less spec is materialized by the bare
// `make_size_descr_with_type_and_vtable` mint rather than
// `simple_descr_group_from_bh_size`, so it never enters
// `gc_cache._cache_size`, `register_unresolved_struct_tids` never walks
// it, and its allocation tid stays the structural hash truncated to
// u32 -- which the walker refuses as `UnregisteredNewGcType` after the
// descent has already run. Registering it here is not the alternative:
// an empty field list carries no ref offsets, so the shape a
// registration would publish says "no pointers" for a struct whose
// size says otherwise, and the collector would stop tracing through it.
// The payload-less sum variant below declines for this same reason.
if let CallTarget::SyntheticTransparentCtor {
is_struct: true, ..
} = target
&& args.is_empty()
&& let ValueType::Ref(Some(owner)) = result_ty
&& self.callcontrol.as_deref().is_some_and(|cc| {
crate::codewriter::assembler::bh_size_spec_from_callcontrol(cc, owner)
.is_some_and(|spec| !spec.all_fielddescrs.is_empty())
})
{
return RewriteResult::Replace(vec![SpaceOperation {
result: op.result.clone(),
kind: OpKind::New {
owner: owner.clone(),
},
}]);
}
// RPython `rtyper` lowers a heap-carried sum-type variant to
// `malloc(GcStruct)` plus its discriminant/payload `setfield`s before
// `jtransform`; `rewrite_op_malloc` then emits `new(descr)`.
Expand Down Expand Up @@ -6319,6 +6377,7 @@ impl<'a> Transformer<'a> {
let CallTarget::SyntheticTransparentCtor {
name,
owner_path: _,
is_struct: _,
} = target
else {
return false;
Expand Down Expand Up @@ -6871,7 +6930,9 @@ fn target_to_call_path(target: &CallTarget) -> crate::parse::CallPath {
crate::parse::CallPath::from_segments(segments.iter().map(String::as_str))
}
CallTarget::Method { name, .. } => crate::parse::CallPath::from_segments([name.as_str()]),
CallTarget::SyntheticTransparentCtor { name, owner_path } => {
CallTarget::SyntheticTransparentCtor {
name, owner_path, ..
} => {
let mut segs: Vec<&str> = owner_path.iter().map(String::as_str).collect();
segs.push(name.as_str());
crate::parse::CallPath::from_segments(segs)
Expand Down
Loading
Loading