tizoc / ocaml-interop

OCaml<->Rust FFI with an emphasis on safety.
MIT License
136 stars 22 forks source link

Bytecode function seems broken #52

Closed mt-caret closed 1 year ago

mt-caret commented 1 year ago

Hi,

The following function:

ocaml_export! {
    fn rust_rust_add_7ints|rust_rust_add_7ints_byte(
            cr,
            int1: OCamlRef<OCamlInt>,
            int2: OCamlRef<OCamlInt>,
            int3: OCamlRef<OCamlInt>,
            int4: OCamlRef<OCamlInt>,
            int5: OCamlRef<OCamlInt>,
            int6: OCamlRef<OCamlInt>,
            int7: OCamlRef<OCamlInt>,
        ) -> OCaml<OCamlInt> {
            let int1: i64 = int1.to_rust(cr);
            let int2: i64 = int2.to_rust(cr);
            let int3: i64 = int3.to_rust(cr);
            let int4: i64 = int4.to_rust(cr);
            let int5: i64 = int5.to_rust(cr);
            let int6: i64 = int6.to_rust(cr);
            let int7: i64 = int7.to_rust(cr);
            unsafe { OCaml::of_i64_unchecked(int1 + int2 + int3 + int4 + int5 + int6 + int7) }
    }
}

Gets expanded to:

#[no_mangle]
pub extern "C" fn rust_rust_add_7ints_byte(
    argv: &[::ocaml_interop::RawOCaml],
    argn: isize,
) -> ::ocaml_interop::RawOCaml {
    let mut i = 0usize;
    let int1 = argv[i];
    i += 1;
    let int2 = argv[i];
    i += 1;
    let int3 = argv[i];
    i += 1;
    let int4 = argv[i];
    i += 1;
    let int5 = argv[i];
    i += 1;
    let int6 = argv[i];
    i += 1;
    let int7 = argv[i];
    i += 1;
    if true {
        if !(i == argn as usize) {
            panic!("assertion failed: i == argn as usize")
        }
    }
    rust_rust_add_7ints(int1, int2, int3, int4, int5, int6, int7)
}

If you were to write this out manually, the Rust compiler gives the following warning:

warning: `extern` fn uses type `[isize]`, which is not FFI-safe
  --> src/lib.rs:27:11
   |
27 |     argv: &[::ocaml_interop::RawOCaml],
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
   |
   = help: consider using a raw pointer instead
   = note: slices have no C equivalent
   = note: `#[warn(improper_ctypes_definitions)]` on by default

And in fact, when actually calling this function from bytecode in OCaml I observe panics like thread '<unnamed>' panicked at 'expected 7 arguments, got 94909550696832'.

I believe we actually want to take a *const ::ocaml_interop::RawOCaml and convert it to a slice using ::std::slice::from_raw_parts(argv, argn as usize).

I hit this issue with my own macros which mirror the expansion behavior of ocaml_export! in ocaml-interop (but with some additional convenience modifications), and hit this issue which I was able to fix with the above change (commit here: https://github.com/mt-caret/polars-ocaml/pull/37/commits/337727bff33d3cede060155d35edfabc812864b0).

tizoc commented 1 year ago

@mt-caret looking into it, but in general, what you say is correct, the generated code should accept a pointer there, then read each value from an offset. Also, argn shouldn't be an isize, but an i32 (just re-checked the OCaml docs and it is an int in C)

mt-caret commented 1 year ago

Aha, interesting, I hadn't considered that aspect. In that case (kind of pedantic, but...), do we want to use this type instead of an i32? https://doc.rust-lang.org/std/os/raw/type.c_int.html

tizoc commented 1 year ago

Just pushed to #53

tizoc commented 1 year ago

Aha, interesting, I hadn't considered that aspect. In that case (kind of pedantic, but...), do we want to use this type instead of an i32? https://doc.rust-lang.org/std/os/raw/type.c_int.html

yes!

tizoc commented 1 year ago

@mt-caret just published v0.9.2 with the fix. Thank you for the debugging work and report.

mt-caret commented 1 year ago

Thanks for the blazing-fast response!! :)