ServoBindingList.h (Servo_*) binding generation isn't type safe

[Manishearth]

When bindgen is run on ServoBindingList.h, it spits out an extern C block full of Servo_* function declarations in bindings.rs

These functions are defined in glue.rs.

There's nothing stopping the declarations from having different signatures from the definitions. Which means that any safety measures we introduce are easy to accidentally circumvent by picking the wrong type.

We should either have a tidy check for this, or have bindgen generate a test file which imports everything from glue and does a type assertion on the function types.

cc @emilio @bholley

[bholley]

Yeah, we get a little bit of safety right now because LLVM crashes if the signatures differ too much (or at least, it did last I checked, which was probably March). :-P But I agree something systematic would be better. Open to doing it however you think is best.

[Manishearth]

Not really sure how to add it to bindgen without adding servo-specific code. We could make it generate tests for functions that share a prefix I guess. Or perhaps tell it to special-case functions found in that specific header file (maybe generate bindings_servo.rs?). We would then need to tell it where to find the functions to compare it with, though a glob import works. Still, it would be highly specialized code.

[bholley]

Can we just fix the rust compiler to complain if there are conflicting definitions of an extern "C" function in the same crate?

[bholley]

(Though I guess the bindings are a different crate - but we could use * them somewhere)

[Manishearth]

I .... don't know.

The "different crate" is a hurdle. Though like I said yesterday, almost all of geckolib (and all of gecko_bindings) could be rolled into style as a single crate.

Its also that we probably aren't using extern decls for their purpose here. You use them to declare functions you plan on linking to, so that you can call them from servo. You don't use them as a "header file"-esque thing to pre-declare functions you plan on defining later. We do this for Servo_* and it's totally unnecessary; it's just that bindgen does it. So I'm not sure if rust would add such a check even in the single-crate world.

[bholley]

Sounds like we're rolling up the crates, so that hurdle is gone.

Even if it's not the intended purpose, it seems reasonable that the rust compiler should check for two conflicting definitions of the same external function in the same crate (and I believe it does so implicitly right now, via the LLVM assert). We should verify that this assert still triggers, and get the rust folks to turn that into a more-useful compiler error.

Also, can you file this in bugzilla as a blocking bug against |stylo| so we don't lose track of it?

[Manishearth]

#13617 gets us the script, and https://bugzilla.mozilla.org/show_bug.cgi?id=1308234 fixes the type issues it found.

However, it doesn't work just yet. The problem is that glue.rs has safe extern C functions, and the functions in geckolib are extern C declarations (which are unsafe even if not explicitly specified).

I made all the Servo_ functions unsafe to find all the mismatched signatures fixed in bug 1308234, but that isn't a permanent solution. The whole point of the FFI safety stuff was to reduce the unsafe surface area; if glue.rs is unsafe-everywhere-by-default we lose out on this.

Sadly casting doesn't work either, Rust doesn't let you cast between fn pointer types. Ideally fn should be a subtype of unsafe fn, but that's not the case either.

This means we need a second version of glue.rs that has all the functions marked as unsafe. This is ... tricky. I tried using a macro with conditional compilation but there are some hurdles in parsing whole functions and putting them back together -- the follow rules don't allow the matchers to work.

The current solution I'm thinking of is to add a build script to tests/unit/stylo which takes glue.rs and generates a second version of the file with all the functions as unsafe, and uses that instead.

[Manishearth]

Interestingly, bindgen picks up the definition of Servo_Node_ClearNodeData from nsINode.h (where it is forward-declared) instead of ServoBindingList.h. @emilio, is this a bug?

[emilio]

It's not, is the same function with the same signature. We only generate the first function we see.

[Manishearth]

The signatures diverge after https://bugzilla.mozilla.org/show_bug.cgi?id=1308234 . Making them the same requires me to forward-declare the borrowed type too, which I can do (I instead moved it to the cpp file)

Ok, if that's expected behavior that's fine.

[emilio]

But they differ just in the typedefs right?

[Manishearth]

Yes. But we ask bindgen to generate bindigns for ServoBindingList.h, and it picks up the signature from the other file. The typedef matters because we rewrite types.