oxidecomputer / idolatry

An experimental IPC interface definition language for Hubris.
Mozilla Public License 2.0
17 stars 11 forks source link

argument types are not always generated in DWARF #12

Open bcantrill opened 2 years ago

bcantrill commented 2 years ago

One of the annoying bits that I have run into is that argument types are not always generated in DWARF. This is no fault of idolatry.

As a concrete example, take this definition:

// Sensor API

Interface(
    name: "Sensor",
    ops: {
        "get": (
            args: {
                "id": (
                    type: "SensorId",
                    recv: From("usize", None),
                )
            },
            reply: Result(
                ok: "f32",
                err: CLike("SensorError"),
            ),
        ),
    },
)

And then this SensorId definition:

#[derive(zerocopy::AsBytes)]
#[repr(C)]
pub struct SensorId(pub usize);

For this definition, a reasonable (that is, functional) server -- that is, one that includes a function that takes SensorId and uses the argument -- does not generate DWARF for SensorId! Because this DWARF is load-bearing with respect to Humility-side HIF generation, not having this type is problematic. Futzing around a bit, there seems to be a zero-cost way of adding this; diff:

diff --git a/src/server.rs b/src/server.rs
index 5cee78e..0ca3756 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -288,20 +288,46 @@ pub fn generate_server_in_order_trait(
     writeln!(out, "        use idol_runtime::ClientError;")?;
     writeln!(out, "        match op {{")?;
     for (opname, op) in &iface.ops {
         writeln!(out, "            {}Operation::{} => {{", iface.name, opname)?;
         writeln!(
             out,
             "                let {}args = read_{}_msg(incoming).ok_or(ClientError::BadMessage)?;",
             if op.args.is_empty() { "_" } else { "" },
             opname
         )?;
+
+        if !op.args.is_empty() {
+            writeln!(out, r##"
+                // The DWARF generated for types is load-bearing in that
+                // Humility potentially needs them to form HIF to make Idol
+                // calls.  Unfortunately, despite the server declaring
+                // functions that take our argument types as parameters, they
+                // are NOT necessarily generated in the DWARF!  To force these
+                // type definitions to be generated, we create meaningless
+                // locals (that themselves will be optimized away); this has
+                // the side-effect of getting the types that we need in the
+                // binary, but without changing the generated instruction
+                // text."##)?;
+
+            for (argname, arg) in &op.args {
+                writeln!(
+                    out,
+                    "                let _arg_{}: Option<&{}> = None;",
+                    argname,
+                    arg.ty.0
+                )?;
+            }
+
+            writeln!(out)?;
+        }
+
         writeln!(out, "                let r = self.1.{}(", opname)?;
         writeln!(out, "                    rm,")?;
         for (argname, arg) in &op.args {
             match &arg.recv {
                 syntax::RecvStrategy::FromBytes => {
                     writeln!(out, "                    args.{},", argname)?;
                 }
                 syntax::RecvStrategy::From(_, None) => {
                     writeln!(
                         out,

This does not change the size of the binary at all (or the instructions generated) but does result in the inclusion of the needed type. (Obviously it would be grand to have a way of doing this that felt less brittle; it would be great -- if complicated -- to add a test for this.)

bcantrill commented 2 years ago

Update on this: as it turns out, the server was not in fact functional -- and the compiler was apparently smart enough to see that and optimize the use of the type away! With that fixed, the DWARF type is present (as expected) -- and without having to get weird on the generated code. I'm going to leave this issue open for the moment, but I'll close it if we don't see another instance of this...