Skip to content

majit: preserve concrete generic struct layouts #1193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Aug 13, 2026
Merged

majit: preserve concrete generic struct layouts #1193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
489 changes: 477 additions & 12 deletions majit/majit-ir/src/descr.rs
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion majit/majit-translate/src/codewriter/assembler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5277,7 +5277,8 @@ mod tests {
);
}
cc.set_struct_fields(struct_fields);
let owner_id = majit_ir::descr::StructId::from_canonical("result::Result");
let template_id = majit_ir::descr::StructId::from_canonical("result::Result");
let owner_id = template_id.instantiate("<i64,PyError>");
let parent_type_id = |owner: &str| {
let field = FieldDescriptor::new("__discriminant", Some(owner.to_string()))
.with_owner_id(Some(owner_id));
Expand Down
69 changes: 61 additions & 8 deletions majit/majit-translate/src/codewriter/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2487,13 +2487,19 @@ impl CallControl {
// publish under the same `struct_key` carries the real
// vtable on its PyreSizeDescr — cache-hit returns
// *that* Arc here unchanged).
let struct_size = compute_struct_size(self, owner_root);
let field_offset = owner_id
.or_else(|| majit_ir::descr::struct_id_for_name(owner_root))
let registry_struct_id = majit_ir::descr::struct_id_for_name(owner_root);
if owner_id.is_some() && registry_struct_id.is_none() {
majit_ir::descr::record_field_owner_id_registry_miss();
}
let (struct_size, struct_size_path) =
compute_struct_size_with_path(self, owner_root);
let exact_field_offset = owner_id
.or(registry_struct_id)
.and_then(|sid| self.struct_layouts.get(&sid))
.and_then(|l| l.fields.iter().find(|f| f.name.as_str() == field_name))
Comment on lines +2496 to 2499

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Fall back after a missing concrete layout

For a generic ADT whose concrete owner_id has not been inserted into struct_layouts, owner_id.or(registry_struct_id) selects that unresolved ID before performing the map lookup, so the available template layout is never tried. Production layout registration only iterates program.struct_fields spellings, and ordinary generic structs remain registered under their unsuffixed template names; their field accesses therefore fall back to the declaration-order offset accumulator. When #[repr(Rust)] reorders or packs such a struct, the generated JIT reads or writes the wrong byte offset instead of using Charon's exact layout. Look up the concrete ID first and then try the registry/template ID, or register every concrete ID before lowering.

AGENTS.md reference: AGENTS.md:L14-L19

Useful? React with 👍 / 👎.

.map(|f| f.offset)
.unwrap_or(offset);
.map(|f| f.offset);
majit_ir::descr::record_field_offset_source(exact_field_offset.is_some());
let field_offset = exact_field_offset.unwrap_or(offset);
let rank = self.field_immutability(Some(owner_root), field_name);
let is_immutable = rank.map(|r| r.is_immutable()).unwrap_or(false);
let is_quasi_immutable = rank.map(|r| r.is_quasi_immutable()).unwrap_or(false);
Expand Down Expand Up @@ -2529,6 +2535,13 @@ impl CallControl {
// layout still has to travel. Read it back off the
// descr rather than off the locals below, which
// describe the mint that did not happen.
trace_field_ei_descr_mint(
"parent_field",
owner_root,
owner_id.is_some(),
registry_struct_id,
struct_size_path,
);
majit_ir::descr::record_ei_descr_mint(
member.clone(),
majit_ir::effectinfo::DescrMintSpec::Field {
Expand Down Expand Up @@ -2585,6 +2598,13 @@ impl CallControl {
// Same arguments this `get_field_descr` miss just used, kept so
// the runtime's own cache can take the same miss branch
// (`descr.py:224-238`) instead of finding an empty slot.
trace_field_ei_descr_mint(
"analyzer_field",
owner_root,
owner_id.is_some(),
registry_struct_id,
struct_size_path,
);
majit_ir::descr::record_ei_descr_mint(
member.clone(),
majit_ir::effectinfo::DescrMintSpec::Field {
Expand Down Expand Up @@ -8065,16 +8085,29 @@ fn field_pos_in(cc: &CallControl, owner: &str, field_name: &str) -> usize {
/// 1. `cc.struct_layouts[struct_name].size` — actual layout
/// 2. Type-string heuristic fallback
fn compute_struct_size(cc: &CallControl, struct_name: &str) -> usize {
compute_struct_size_with_path(cc, struct_name).0
}

fn compute_struct_size_with_path(
cc: &CallControl,
struct_name: &str,
) -> (usize, majit_ir::descr::StructSizePath) {
// Path 1: actual layout from runtime (RPython: symbolic.get_size(STRUCT))
if let Some(layout) = cc.struct_layout_for(struct_name) {
return layout.size;
let path = majit_ir::descr::StructSizePath::Layout;
majit_ir::descr::record_compute_struct_size_path(path);
return (layout.size, path);
}
// Path 2: heuristic fallback — RPython: symbolic always computes the full
// struct size, even with nested structs. Nested struct sizes are looked up
// recursively from struct_layouts.
let fields = match cc.struct_fields.fields.get(struct_name) {
Some(f) => f,
None => return 0,
None => {
let path = majit_ir::descr::StructSizePath::FieldsMissing;
majit_ir::descr::record_compute_struct_size_path(path);
return (0, path);
}
};
let mut offset: usize = 0;
for (_, field_type_str) in fields.iter() {
Expand Down Expand Up @@ -8109,10 +8142,30 @@ fn compute_struct_size(cc: &CallControl, struct_name: &str) -> usize {
.filter(|s| *s > 0)
.max()
.unwrap_or_else(crate::layout::target_word_size);
if offset > 0 {
let size = if offset > 0 {
(offset + max_align - 1) & !(max_align - 1)
} else {
0
};
let path = majit_ir::descr::StructSizePath::Heuristic;
majit_ir::descr::record_compute_struct_size_path(path);
(size, path)
}

fn trace_field_ei_descr_mint(
site: &str,
owner_root: &str,
owner_id_is_some: bool,
registry_struct_id: Option<majit_ir::descr::StructId>,
struct_size_path: majit_ir::descr::StructSizePath,
) {
if majit_ir::descr::field_mint_trace_enabled() {
eprintln!(
"MAJIT_FIELD_MINT_TRACE ei_descr_mint site={site} owner_root={owner_root:?} \
owner_id_is_some={owner_id_is_some} \
struct_id_for_name={registry_struct_id:?} \
compute_struct_size_path={struct_size_path:?}"
);
}
}

Expand Down
102 changes: 78 additions & 24 deletions majit/majit-translate/src/front/mir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,27 @@ struct RefEnumInst {
suffix: String,
}

/// Physical layout identity for a concrete ADT use. The defining path supplies
/// the template identity; all rendered type arguments supply the
/// monomorphization identity. This is deliberately broader than
/// `adt_head_instantiation_suffix`, whose reference-payload predicate controls
/// annotator class splitting rather than Rust memory layout.
fn concrete_adt_struct_id(
template: majit_ir::descr::StructId,
adt: Option<&serde_json::Map<String, serde_json::Value>>,
llbc: &Llbc,
) -> majit_ir::descr::StructId {
let Some(adt) = adt else {
return template;
};
let args = render_adt_type_args(adt, llbc, 0);
if args.is_empty() {
template
} else {
template.instantiate(&format!("<{}>", args.join(",")))
}
}

/// The [`RefEnumInst`] for the ADT descriptor `adt` (`{"id": …,
/// "generics": …}`) when it names a split-eligible reference-payload
/// generic enum, else `None`. Shared by both instantiation scans below so
Expand Down Expand Up @@ -4783,9 +4804,9 @@ impl<'a> Lowering<'a> {
// use `variant_idx = null`, enum variants index into the
// `TypeDeclKind::Enum` variant list.
let resolved = self.resolve_aggregate_adt(&kind);
let (owner_path, ctor_name, field_names) = match resolved {
Some((owner_path, ctor_name, field_names)) => {
(owner_path, ctor_name, field_names)
let (owner_path, ctor_name, field_names, aggregate_owner_id) = match resolved {
Some((owner_path, ctor_name, field_names, owner_id)) => {
(owner_path, ctor_name, field_names, Some(owner_id))
}
None => {
// Synthetic placeholders for non-Adt aggregates
Expand All @@ -4811,7 +4832,7 @@ impl<'a> Lowering<'a> {
);
let positional =
(0..arg_vars.len()).map(|i| format!("__pos_{i}")).collect();
(Vec::new(), leaf, positional)
(Vec::new(), leaf, positional, None)
}
};
let result_ty_owner = if owner_path.is_empty() {
Expand Down Expand Up @@ -4899,7 +4920,7 @@ impl<'a> Lowering<'a> {
field: crate::model::FieldDescriptor {
name,
owner_root: Some(result_ty_owner.clone()),
owner_id: None,
owner_id: aggregate_owner_id,
base_is_deref: None,
taken_by_address: false,
},
Expand Down Expand Up @@ -4931,15 +4952,16 @@ impl<'a> Lowering<'a> {
// sites project, so the `__discriminant` read and the
// constructor's `setattr` land on ONE classdef per
// instantiation (`enum_variant_narrowing_knowntypedata`
// then mints matching variant subclasses). `owner_id` (the
// layout-side `StructId`) stays on the bare template name so
// every instantiation shares the one template tag layout —
// `from_canonical` keys the un-suffixed path.
// then mints matching variant subclasses). `owner_id` is the
// independent physical-layout identity and preserves every
// concrete generic argument.
let (owner_root, owner_id) = match self.tyref_adt_class_root(&place.ty) {
Some(class_root) => {
let canon = strip_crate_prefix(&class_root);
let bare = majit_ir::descr::strip_instantiation_suffix(&canon);
let sid = majit_ir::descr::StructId::from_canonical(bare);
let sid = self.tyref_adt_layout_id(&place.ty).unwrap_or_else(|| {
let bare = majit_ir::descr::strip_instantiation_suffix(&canon);
majit_ir::descr::StructId::from_canonical(bare)
});
(Some(canon), Some(sid))
}
None => (None, None),
Expand Down Expand Up @@ -5924,7 +5946,7 @@ impl<'a> Lowering<'a> {
fn resolve_aggregate_adt(
&self,
kind: &serde_json::Value,
) -> Option<(Vec<String>, String, Vec<String>)> {
) -> Option<(Vec<String>, String, Vec<String>, majit_ir::descr::StructId)> {
let adt = kind.as_object()?.get("Adt")?.as_array()?;
// `AggregateKind::Adt` head: either a bare `type_id` u64 or a
// full `TypeDeclRef` object `{"generics": …, "id": {"Adt":
Expand All @@ -5944,6 +5966,7 @@ impl<'a> Lowering<'a> {
let variant_idx = adt.get(1).and_then(serde_json::Value::as_u64);
let td = self.llbc.type_by_id(type_id)?;
let name_path = td.item_meta.name_path();
let head_adt = head.as_object();
let mut segments: Vec<String> = name_path.split("::").map(str::to_string).collect();
let type_leaf = segments.pop().unwrap_or_default();
let owner_path = segments;
Expand All @@ -5954,7 +5977,14 @@ impl<'a> Lowering<'a> {
.enumerate()
.map(|(i, f)| f.name.clone().unwrap_or_else(|| format!("__pos_{i}")))
.collect();
Some((owner_path, type_leaf, field_names))
let template =
majit_ir::descr::StructId::from_canonical(&strip_crate_prefix(&name_path));
Some((
owner_path,
type_leaf,
field_names,
concrete_adt_struct_id(template, head_adt, self.llbc),
))
}
(TypeDeclKind::Enum(variants), Some(idx)) => {
let v = variants.get(idx as usize)?;
Expand All @@ -5980,7 +6010,17 @@ impl<'a> Lowering<'a> {
.enumerate()
.map(|(i, f)| f.name.clone().unwrap_or_else(|| format!("__pos_{i}")))
.collect();
Some((variant_owner, v.name.clone(), field_names))
let template = majit_ir::descr::StructId::from_canonical(&format!(
"{}::{}",
strip_crate_prefix(&name_path),
v.name
));
Some((
variant_owner,
v.name.clone(),
field_names,
concrete_adt_struct_id(template, head_adt, self.llbc),
))
}
_ => None,
}
Expand Down Expand Up @@ -6058,6 +6098,7 @@ impl<'a> Lowering<'a> {
// variant field read, so the per-instantiation variant class the
// constructor and receiver project had no matching field read.
let head = adt.first()?;
let head_adt = head.as_object();
let type_id = match head.as_u64() {
Some(id) => id,
None => head.get("id")?.get("Adt")?.as_u64()?,
Expand All @@ -6075,11 +6116,10 @@ impl<'a> Lowering<'a> {
// `owner_root` is the annotation-side classdef key: a
// reference-payload workspace enum instantiation reads its field
// off the per-instantiation variant class (`Result<Tuple>::Ok`),
// matching the receiver / constructor projection. `owner_id`
// (the layout-side `StructId` minted below) stays on the bare
// template name so every instantiation shares the one template
// variant layout — sound because the split is scoped to
// reference payloads, which all share that word-slot layout.
// matching the receiver / constructor projection. `owner_id` is the
// independent layout-side identity and preserves every concrete type
// argument, including primitive payloads for which the annotator does
// not split classdefs.
let owner_leaf = name_path.rsplit("::").next().unwrap_or("").to_string();
let owner_root = match head
.as_object()
Expand All @@ -6096,9 +6136,9 @@ impl<'a> Lowering<'a> {
.clone()
.unwrap_or_else(|| format!("__pos_{field_idx}"));
let ty = clone_tyref(&f.ty);
let owner_id = Some(majit_ir::descr::StructId::from_canonical(
&strip_crate_prefix(&name_path),
));
let template =
majit_ir::descr::StructId::from_canonical(&strip_crate_prefix(&name_path));
let owner_id = Some(concrete_adt_struct_id(template, head_adt, self.llbc));
Some((owner_root, name, ty, owner_id))
}
(TypeDeclKind::Enum(variants), Some(vidx)) => {
Expand All @@ -6115,11 +6155,12 @@ impl<'a> Lowering<'a> {
// registered under this key). The downcast statically fixes
// the variant.
let variant_owner = format!("{owner_root}::{}", variant.name);
let owner_id = Some(majit_ir::descr::StructId::from_canonical(&format!(
let template = majit_ir::descr::StructId::from_canonical(&format!(
"{}::{}",
strip_crate_prefix(&name_path),
variant.name
)));
));
let owner_id = Some(concrete_adt_struct_id(template, head_adt, self.llbc));
Some((variant_owner, name, ty, owner_id))
}
_ => None,
Expand Down Expand Up @@ -12883,6 +12924,19 @@ impl<'a> Lowering<'a> {
}
}

/// Concrete physical-layout identity of an ADT type. Unlike
/// [`Self::tyref_adt_class_root`], this always preserves generic arguments:
/// annotator class splitting is selective, while Rust monomorphization is
/// not.
fn tyref_adt_layout_id(&self, ty: &TyRef) -> Option<majit_ir::descr::StructId> {
let value = self.tyref_adt_body(ty)?;
let def_id = inline_adt_def_id(value)?;
let name_path = self.llbc.type_by_id(def_id)?.item_meta.name_path();
let template = majit_ir::descr::StructId::from_canonical(&strip_crate_prefix(&name_path));
let adt = value.as_object()?.get("Adt")?.as_object();
Some(concrete_adt_struct_id(template, adt, self.llbc))
}

/// `true` when `ty` resolves to a FIELDLESS enum whose discriminant
/// tag sits at the value's base (byte 0). Mirrors the
/// [`Lowering::tyref_adt_name_path`] resolution (dedup / hash-consed
Expand Down
Loading
Loading