diku-dk / futhark

:boom::computer::boom: A data-parallel functional programming language
http://futhark-lang.org
ISC License
2.38k stars 165 forks source link

Array size not in manifest #2070

Open dcz-self opened 9 months ago

dcz-self commented 9 months ago

When a function has a fixed-length array argument like (height: [16]i32), the manifest does not reflect that, and instead suggests that the array is dynamic length: []i32. This prevents wrappers from verifying that the arrays received are of the correct size, and results in the code simply segfaulting. Noticed with cargo-futhark.

athas commented 9 months ago

This should not be a segfault; it should be a proper dynamically checked error. Does it really segfault with cargo-futhark?

It would be nice to provide some more information like this in the manifest, but it's not immediately straightforward what it should look like. We would like to support all possible invariants, such as requiring two arrays that have the same size (but is not otherwise constrained). One solution would be to just provide the actual Futhark type, and let sufficiently smart bridges figure it out.

dcz-self commented 9 months ago

Just checked again and got this backtrace:

#0  0x0000555555561583 in futhark_multicore_multicore__memblock_unref (ctx=0x5555555bec10, block=0x0, 
    desc=0x5555555a712e "arr->mem")
    at /foo/target/debug/build/rain-2f80d9ff3266146f/out/futhark/multicore/futhark_lib.c:6411
#1  0x00005555555648ed in futhark_multicore_free_u8_2d (ctx=0x5555555bec10, arr=0x0)
    at /foo/target/debug/build/rain-2f80d9ff3266146f/out/futhark/multicore/futhark_lib.c:7601
#2  0x000055555555dbb4 in rain::backends::multicore::{impl#0}::futhark_free_u8_2d (ctx=0x5555555bec10, arr=0x0)
    at target/debug/build/rain-2f80d9ff3266146f/out/futhark/futhark_lib.rs:375
#3  0x000055555555ce99 in rain::{impl#13}::drop<rain::backends::multicore::MultiCore> (self=0x7fffffffd280)
    at target/debug/build/rain-2f80d9ff3266146f/out/futhark/futhark_lib.rs:777
#4  0x000055555555ce1a in core::ptr::drop_in_place<rain::Array_U8_2D<rain::backends::multicore::MultiCore>> ()
    at /builddir/build/BUILD/rustc-1.74.0-src/library/core/src/ptr/mod.rs:497
#5  0x000055555555d124 in rain::Context<rain::backends::multicore::MultiCore>::entry_rain<rain::backends::multicore::MultiCore> (self=0x7fffffffd378, id_ids=0x7fffffffd3a8, id_colors=0x7fffffffd3b8, color=0x7fffffffd3c8, height=0x7fffffffd3d8, 
    blocks=0x7fffffffd3e8) at target/debug/build/rain-2f80d9ff3266146f/out/futhark/futhark_lib.rs:114
#6  0x000055555555cabd in futest::main () at src/bin/futest.rs:15

Using current git futhark-cargo (crates version is broken).

fn main() {
    let ctx = rain::Context::<rain::backends::MultiCore>::new(rain::Config::new());
    ctx.entry_rain(
        &rain::Array_U16_1D::new(&ctx, &[0], 1),
        &rain::Array_U8_2D::new(&ctx, &[0,0,0,0], 1, 4),
        &rain::Array_U8_2D::new(&ctx, &[0;4], 1, 4), // <- this is mismatched
        &rain::Array_I32_1D::new(&ctx, &[0;256], 16 * 16),
        &rain::Array_U16_1D::new(&ctx, &[0;4096], 4096)
    ).unwrap();
    ctx.sync();
}
type color = [4]u8
type blockcolors = [16*16]color
entry rain
    (id_ids: []u16)
    (id_colors: [][4]u8)
    (color: blockcolors)
    (height: [16*16]i32)
    (blocks: [16*16*16]u16)
: blockcolors = [...]
dcz-self commented 9 months ago

Speaking as an application developer, having a basic constraint describing only fixed size arrays would already be great. If not the binding, then I have to implement the convention by hand anyway.

athas commented 9 months ago

How is that Rust code type correct? The third argument to the entry point must be a one-dimensional array, but you pass it a two-dimensional array.

athas commented 9 months ago

Wait, no, I misread.

athas commented 9 months ago

I am still confused, however. What does rain::Array_U8_2D::new(&ctx, &[0;4], 1, 4) produce? What is [0;4], and are you trying to create an array of shape [1][4] from it?

dcz-self commented 9 months ago

&[0;4] is another syntax for &[0,0,0,0], which creates a contiguous area of 4 bytes. There are no native multidimensional arrays in Rust, so the pointer gets passed together with the array dimensions without much modifications to the C side of the binding.

I presume rain::Array_U8_2D::new() is just wrapped futhark_multicore_new_u8_2d(), so it produces a handle to the struct.

dcz-self commented 9 months ago

Looking at the backtrace again, the problem appears in Drop/free. I think this makes it quite likely that the bindings are incorrect. I'll take a closer look when I have the time.

athas commented 9 months ago

My guess is that the entry point fails (as it should), meaning the array it returns (at the C level) is uninitialised, which is not handled properly by cargo-futhark.

dcz-self commented 9 months ago

Adding a null pointer check seems to work. It's surprising to see that the output array is allocated in the call to the entry function, but I guess valid. Typically C interfaces expect the caller to provide a pre-allocated array.

athas commented 9 months ago

Typically C interfaces expect the caller to provide a pre-allocated array.

That would only be possible if the C API exposed the size of the various structs, which raises all kinds of problems. It's an important feature that all allocation is done by Futhark.

dcz-self commented 9 months ago

It would be nice to provide some more information like this in the manifest, but it's not immediately straightforward what it should look like. We would like to support all possible invariants, such as requiring two arrays that have the same size (but is not otherwise constrained). One solution would be to just provide the actual Futhark type, and let sufficiently smart bridges figure it out.

What requirements should a proposed solution fulfill? You already listed all constraints I'm aware of :)

How does providing the Futhark type help when 2 arrays are to have the same size?

athas commented 9 months ago

A futhark function type

[n]i32 -> [n]bool -> ...

explicitly expresses that the two arguments must have the same shape. If n is a size parameter, that means the size can be anything.

The question is how it should be encoded in the manifest. The type field for entry point parameters/results refers to type names (as in the other part of the manifest), which does not (and cannot) include sizes. We could add an type_elaborated field that contains the original Futhark-level type as a string.