Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bytecode function seems broken #52

Closed
mt-caret opened this issue Aug 27, 2023 · 6 comments · Fixed by #53
Closed

Bytecode function seems broken #52

mt-caret opened this issue Aug 27, 2023 · 6 comments · Fixed by #53

Comments

@mt-caret
Copy link
Contributor

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: mt-caret/polars-ocaml@337727b).

@tizoc
Copy link
Owner

tizoc commented Aug 27, 2023

@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
Copy link
Contributor Author

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
Copy link
Owner

tizoc commented Aug 27, 2023

Just pushed to #53

@tizoc
Copy link
Owner

tizoc commented Aug 27, 2023

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 tizoc closed this as completed in #53 Aug 27, 2023
@tizoc
Copy link
Owner

tizoc commented Aug 27, 2023

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

@mt-caret
Copy link
Contributor Author

Thanks for the blazing-fast response!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants