futhark icon indicating copy to clipboard operation
futhark copied to clipboard

Array size not in manifest

Open dcz-self opened this issue 1 year ago • 13 comments

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.

dcz-self avatar Jan 06 '24 18:01 dcz-self

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.

athas avatar Jan 06 '24 21:01 athas

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 avatar Jan 06 '24 22:01 dcz-self

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.

dcz-self avatar Jan 06 '24 22:01 dcz-self

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 avatar Jan 06 '24 22:01 athas

Wait, no, I misread.

athas avatar Jan 06 '24 22:01 athas

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?

athas avatar Jan 06 '24 22:01 athas

&[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 avatar Jan 06 '24 22:01 dcz-self

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.

dcz-self avatar Jan 06 '24 22:01 dcz-self

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.

athas avatar Jan 06 '24 22:01 athas

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.

dcz-self avatar Jan 06 '24 22:01 dcz-self

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.

athas avatar Jan 07 '24 08:01 athas

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?

dcz-self avatar Jan 07 '24 20:01 dcz-self

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.

athas avatar Jan 07 '24 21:01 athas