native-lib mode does not support null function pointers passed as `Option<fn()>`

[weiznich]

I believe that this is more a feature request than anything else. I've tried again running some of diesels tests via miri and with the Zmiri-native-lib support I think it should be finally nearly possible now.

The remaining roadblock seems to be errors like the following:

error: unsupported operation: passing a non-#[repr(C)] struct over FFI: std::option::Option<unsafe extern "C" fn(*mut std::ffi::c_void, i32, *mut *mut i8, *mut *mut i8) -> i32>
   --> /home/weiznich/Documents/rust/diesel/diesel/src/sqlite/connection/raw.rs:75:13
    |
 75 | /             ffi::sqlite3_exec(
 76 | |                 self.internal_connection.as_ptr(),
 77 | |                 query.as_ptr(),
 78 | |                 callback_fn,
 79 | |                 callback_arg,
 80 | |                 ptr::null_mut(),
 81 | |             )
    | |_____________^ unsupported operation occurred here

Relevant code link: https://github.com/diesel-rs/diesel/blob/ed01866ec0bf63070d179da4ce3d37df582f3e79/diesel/src/sqlite/connection/raw.rs#L71-L82

The sqlite3_exec function is provided by libsqlite3-sys as a generated binding for the underlying sqlite3_exec function from sqlite itself. These bindings are generated by bindgen, so my expectation here would be that miri is able to deal with bindings generated by bindgen or that bindgen generates bindings that are accepted by miri. Right of now there seems to be some sort of disagreement of what's accepted and what not.

Technically speaking I believe the code generated by bindgen is actually sound as the documentation of Option explicitly calls out that specifically Option<unsafe extern "C" fn (…)>::None is equivalent to a null function pointer.

So this is more or less the request to support "non #[repr(C)]" rust types with an explicitly specified representation in FFI code.

[RalfJung]

Yeah, Miri's native-lib mode does not yet support all types that are actually ABI-safe.

[RalfJung]

Looking at this again, I don't think the Option wrapper is the main problem here... this is a function pointer! We're nowhere close to supporting function pointers across the FFI boundary. Having C code call back into Rust code is, ugh, somewhere between totally impossible and extremely difficult. I have no idea how it could be done but @nia-e may have some crazy ideas. ;)

It looks like in this particular case, the function pointer is null, but it would be non-trivial to ensure that we only allow passing function pointers if they are null.

[nia-e]

I do happen to have some crazy ideas in the pipeline that can totally make cross language callbacks work ^^ though they're a decent ways from getting written, since I haven't had time to work on this much recently. Happy to share design notes if it's relevant, though

[RalfJung]

@weiznich FWIW I would be quite surprised if sqlite just works now, I assume there's quite a few missing bits and we have to fix one to even learn what the next problem is. Please do feel free to keep reporting these, this is very useful feedback to figure out what is needed for real-world usage of this feature. :)

[weiznich]

Thanks for the fast fix and also for happily accepting these reports. I will try to continue get this running. How long does it usually take until a fix like this is in the next nightly release?

[RalfJung]

Usually I sync on weekends. I can do extra syncs if there's demand. Alternatively, it's also not that hard to build Miri from source if you want to try that. ;)

cargo install rustup-toolchain-install-master
./miri toolchain && ./miri install
# then in a different folder
cargo +miri miri test

[RalfJung]

Sync landed in rust-lang/rust#147744.