feat: preserve ABI types#1151
Conversation
|
NOTE: For this to work end-to-end, we need a PR in VM, but since compiler is not migrated yet here is branch based on the latest supported version: https://github.com/walnuthq/miden-vm/tree/pr/debug-type-names-v0.22.3 (without this patch, we would see anonymous structs). Basically, there we salvage ABI type names from user level into the $ miden-objtool dump debug-info
...
[ 10] STRUCT miden:base/core-types@1.0.0/account-id (size: 8 bytes)
+ 0: prefix : struct miden:base/core-types@1.0.0/felt
+ 4: suffix : struct miden:base/core-types@1.0.0/felt |
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
I think this is ok, but we really need integration test coverage here to make sure we don't end up encoding inaccurate signatures that we then later rely on for binding generation, etc.
| .map(|result| masm::TypeExpr::from(result.ty.clone())) | ||
| .collect(); | ||
| let signature = masm::FunctionType::new(sig.cc, args, results); | ||
| let signature = |
There was a problem hiding this comment.
We need to make sure that this signature is the same as the Component ABI signature when the calling convention is component-model, or this signature will not be correct.
We at the very least need an integration test with a mixture of types that asserts that the ABI expressed by the "semantic" signature here, and the one actually expressed at the Component ABI level are the same, so that we can catch drift between them.
I'm expecting that in general we'll be relying on WIT for this anyway once I finish up the binding generation work - but I'm fine with the signature we encode in MASM/package metadata to match the Component ABI types, since we must always derive the flattened representation for codegen anyway.
There was a problem hiding this comment.
Thanks! It is addressed!
greenhat
left a comment
greenhat
left a comment
There was a problem hiding this comment.
Looking good overall! Please see my comments.
| .with_span(SourceSpan::UNKNOWN), | ||
| ) | ||
| } | ||
| Type::Ptr(ptr) => masm::TypeExpr::Ptr(masm::PointerType::new( |
There was a problem hiding this comment.
PointerType::new defaults to the element address space. Should we preserve ptr's address space?
Also, we have a similar (to masm_component_type_expr_from_hir) conversion already defined in miden-assembly-syntax:
impl From<Type> for TypeExpr {
fn from(ty: Type) -> Self {
match ty {
Type::Array(t) => Self::Array(ArrayType::new(t.element_type().clone().into(), t.len())),
Type::Struct(t) => Self::Struct(StructType::new(
None,
t.fields().iter().enumerate().map(|(i, ft)| {
let name = Ident::new(format!("field{i}")).unwrap();
StructField {
span: SourceSpan::UNKNOWN,
name,
ty: ft.ty.clone().into(),
}
}),
)),
Type::Ptr(t) => Self::Ptr((*t).clone().into()),
Type::Function(_) => {
Self::Ptr(PointerType::new(TypeExpr::Primitive(Span::unknown(Type::Felt))))
},
Type::List(t) => Self::Ptr(
PointerType::new((*t).clone().into()).with_address_space(AddressSpace::Byte),
),
Type::I128 | Type::U128 => Self::Array(ArrayType::new(Type::U32.into(), 4)),
Type::I64 | Type::U64 => Self::Array(ArrayType::new(Type::U32.into(), 2)),
Type::Unknown | Type::Never | Type::F64 => panic!("unrepresentable type value: {ty}"),
ty => Self::Primitive(Span::unknown(ty)),
}
}
}There was a problem hiding this comment.
Yes, nice catch! Thanks!!
| 0, | ||
| body, | ||
| ); | ||
| start.extend_invoked(invoked); |
There was a problem hiding this comment.
The component export wrappers inject init and truncate_stack with raw Exec ops. We should track them as well.
compiler/codegen/masm/src/lower/component.rs
Line 764 in 3c03b01
compiler/codegen/masm/src/lower/component.rs
Line 744 in 3c03b01
greenhat
left a comment
greenhat
left a comment
There was a problem hiding this comment.
Please see my note regarding the conversions.
| Some(masm::FunctionType::new(ty.calling_convention(), args, results)) | ||
| } | ||
|
|
||
| fn masm_component_type_expr_from_hir(ty: &Type) -> masm::TypeExpr { |
There was a problem hiding this comment.
I checked again and I did not find any difference between this conversion and the one I posted above from the miden-assembly-syntax crate. Is there any reason for not using the one from miden-assembly-syntax?
There was a problem hiding this comment.
the thing is that there is a difference, see, like
TypeExpr::from(Type::U64) becomes [u32; 2],
and the function I am adding (now renamed) component_abi_type_expr_from_hir(Type::U64) stays u64
| } | ||
| } | ||
|
|
||
| fn masm_type_expr_from_hir(ty: &Type) -> masm::TypeExpr { |
There was a problem hiding this comment.
This conversion differs from the masm_component_type_expr_from_hir only by handling I64 and I128 types.
There was a problem hiding this comment.
yeah, it is a duplicate, leftover -- I locally handled it
|
can we merge this one? :) |
3 participants
Typed ABI/debug consumers need to distinguish the lowered execution ABI from the source-level/component ABI. Without preserving the semantic signature, tools only see flattened
feltparameters/results and cannot recover types likeaccount-id,word, orasset.