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

Array size not in manifest #2070

Open
dcz-self opened this issue Jan 6, 2024 · 13 comments
Open

Array size not in manifest #2070

dcz-self opened this issue Jan 6, 2024 · 13 comments

Comments

@dcz-self
Copy link
Contributor

dcz-self commented Jan 6, 2024

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

athas commented Jan 6, 2024

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

dcz-self commented Jan 6, 2024

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

dcz-self commented Jan 6, 2024

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

athas commented Jan 6, 2024

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

athas commented Jan 6, 2024

Wait, no, I misread.

@athas
Copy link
Member

athas commented Jan 6, 2024

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

dcz-self commented Jan 6, 2024

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

dcz-self commented Jan 6, 2024

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

athas commented Jan 6, 2024

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

dcz-self commented Jan 6, 2024

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

athas commented Jan 7, 2024

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

dcz-self commented Jan 7, 2024

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

athas commented Jan 7, 2024

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.

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

No branches or pull requests

2 participants