Re: [RFC PATCH 07/25] tools/xenbindgen: Add support for structs in TOML specs

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Mon Nov 25, 2024 at 12:39 PM GMT, Teddy Astie wrote:
> Hi Alejandro,
>
> Le 15/11/2024 à 12:51, Alejandro Vallejo a écrit :
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> > ---
> >   tools/rust/xenbindgen/src/c_lang.rs | 56 ++++++++++++++++++++++++-
> >   tools/rust/xenbindgen/src/spec.rs   | 64 ++++++++++++++++++++++++++++-
> >   2 files changed, 117 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/rust/xenbindgen/src/c_lang.rs 
> > b/tools/rust/xenbindgen/src/c_lang.rs
> > index f05e36bb362f..597e0ed41362 100644
> > --- a/tools/rust/xenbindgen/src/c_lang.rs
> > +++ b/tools/rust/xenbindgen/src/c_lang.rs
> > @@ -17,9 +17,10 @@
> >
> >   use std::fmt::Write;
> >
> > -use crate::spec::OutFileDef;
> > +use crate::spec::{OutFileDef, StructDef, Typ};
> >
> >   use convert_case::{Case, Casing};
> > +use log::{debug, trace};
> >
> >   /// An abstract indentation level. 0 is no indentation, 1 is 
> > [`INDENT_WIDTH`]
> >   /// and so on.
> > @@ -29,6 +30,39 @@ struct Indentation(usize);
> >   /// Default width of each level of indentation
> >   const INDENT_WIDTH: usize = 4;
> >
> > +/// Create a C-compatible struct field. Without the terminating semicolon.
> > +fn structfield(typ: &Typ, name: &str) -> String {
> > +    match typ {
> > +        Typ::Ptr(x) => {
> > +            let t: &Typ = x;
> > +            format!(
> > +                "XEN_GUEST_HANDLE_64({}) {name}",
> > +                match t {
> > +                    Typ::U8 => "uint8",
> > +                    Typ::U16 => "uint16",
> > +                    Typ::U32 => "uint32",
> > +                    Typ::U64 => "uint64_aligned_t",
> > +                    Typ::I8 => "int8",
> > +                    Typ::I16 => "int16",
> > +                    Typ::I32 => "int32",
> > +                    Typ::I64 => "int64_aligned_t",
> > +                    _ => panic!("foo {t:?}"),
> > +                }
> > +            )
> > +        }
> > +        Typ::Struct(x) => format!("struct {x} {name}"),
> > +        Typ::Array(x, len) => format!("{}{name}[{len}]", structfield(x, 
> > "")),
> > +        Typ::U8 => format!("uint8_t {name}"),
> > +        Typ::U16 => format!("uint16_t {name}"),
> > +        Typ::U32 => format!("uint32_t {name}"),
> > +        Typ::U64 => format!("uint64_aligned_t {name}"),
> > +        Typ::I8 => format!("int8_t {name}"),
> > +        Typ::I16 => format!("int16_t {name}"),
> > +        Typ::I32 => format!("int32_t {name}"),
> > +        Typ::I64 => format!("int64_aligned_t {name}"),
> > +    }
> > +}
> > +
>
> I think _t are missing in the Ptr cases (we are currently generating
> XEN_GUEST_HANDLE_64(uint8) which I don't think is valid).

It is intentional. The handles are presently missing those _t in Xen's public
headers, but that's something I'll be changing in the interest of sanity. That
way we can just recurse to the inner type.


> Aside that, wouldn't it be better to have a separate function for
> converting the type to its C representation ?
>
> Something like
>
> impl Typ { // or blanket trait
>      fn c_repr(&self) -> String {
>          match self {
>              /* ... */
>          }
>      }
> }

That's roughhly what typ_rs() does, and indeed what typ_c() used to do. There's
a complication though...

>
> fn structfield(typ: &Typ, name: &str) -> String {
>      format!("{} {name}", typ.c_repr());
> }
>
> We can also consider Typ::Struct or Typ::Array cases to call recursively
> to c_repr on its inner type to get its representation.
>
> That way, we can have XEN_GUEST_HANDLE_64(struct something).

Initially structfield() was typ_c() (like the Rust backend). Then arrays
happened... Size and typename surround the name of the field (e.g: uint8_t
handle[16]) so I stopped doing it like that because I thought I couldn't.

I have since then noticed I can cheat! The following two fields are identical.
Except the first one is a heck of a lot simpler to generate.

  __typeof__(uint8_t[16]) handle;
  uint8_t handle[16];

My latest branch simplifies all this by s/structfield/typ_c/ and using that
typeof trick.

>
> Cheers
>
> Teddy
>
>
>
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech

Cheers,
Alejandro