search

ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.19k stars 1.06k forks source link

runtime/array.c: blitting from flat to non-flat is not supported #13102

Closed gasche closed 3 days ago

gasche commented 4 weeks ago

In general, to work with uniform arrays it is enough to ensure that the arrays we consider are always initialized with a non-float value. (Context: https://github.com/ocaml/ocaml/pull/12885 , https://github.com/ocaml/RFCs/pull/37 )

In the process of reviewing #12885, @OlivierNicole found what could be considered an exception to this rule: it is invalid to call Array.blit on non-flat-float destination array if the source array is itself a flat-float array.

This PR minimally documents this fact by adding a CAMLassert call in caml_array_blit (the C implementation of Array.blit in the runtime) that checks that the source array is not a flat float array. (The implementation is not correct in this case.) This would have sufficed to find the issue in #12885, as the CI runs the testsuite in a debug build.

(One might argue that Array.blit should support this use-case of blitting from flat-float to non-flat-float array, but this is more work and it only matters for unsafe applications that can also be careful and/or avoid Array.blit entirely.)

cc @OlivierNicole and also @xavierleroy

xavierleroy commented 4 weeks ago

I don't think non-flat to flat works either, so while you're at it, there's one more assertion to add.

gasche commented 4 weeks ago

Good point, done!

gasche commented 4 weeks ago

CI report (debug build):

List of failed tests:
    tests/lib-floatarray/floatarray.ml

Looking at it.

gasche commented 4 weeks ago

The new assertions are not correct in the case of size-0 arrays, because size-0 floatarrays are represented as atoms with tag 0 and not with tag Double_flat_float.

I made the assertions correct (assuming that the len parameter of the blit is valid) by adding an early exit in the len==0 case, which in particular rules out size-0 arrays.

Note: Size-0 blits occur in the wild due to the implementation of Float.Array.append which calls blit twice, so that appending an empty float array will call blit with a size-0 source. (Using caml_array_gather directly for Float.Array.append would be slightly more efficient as blit uses a memory barrier which is not needed for append, but oh well.)