Add Dynarray.unsafe_to_iarray

[OlivierNicole]

Fixes #13793.

[gasche]

You should steal the backing array from the dynarray, which can be done by calling reset a instead of a_.length <- 0. If you do this, then there is no problem with f leaking a, the leaked aliases will start operating on an independent backing array.

[gasche]

So here would be a suggested implementation (untested):

let unsafe_to_iarray ~capacity (f : 'a t -> unit) =
  let a = create () in
  set_capacity a capacity;
  f a;
  let Pack {arr; length; dummy} = a in
  reset a;
  (* at this point further updates to [a] will not mutate [arr]. *)
  check_valid_length length arr;
  let arr_values : ('a, _) with_dummy =
    if Array.length arr = length then arr
    else Dummy.Array.prefix ~dummy arr length
  in
  let values : 'a array =
    try Dummy.Array.unsafe_nocopy_to_array ~dummy arr_values
    with Dummy_found i -> Error.missing_element ~i ~length
  in
  unsafe_iarray_of_array values

[gasche]

(Note: I edited the version above slightly after the fact, so maybe don't rely on what is in your email reader.)

[OlivierNicole]

Using reset to replace the backing array is a good idea, thanks.

[gasche]

I would propose to rename the existing copy and unsafe_nocopy functions into {unsafe_no,}copy_from_array, and call your new function unsafe_nocopy_to_array.

[gasche]

Assumes [...] that none of its elements are dummies

I don't see a good reason to do this, you should check that none of the elements are dummies and fail with Invalid_argument if you find one. (I would do this by having a custom exception Dummy_found of int in this low-level module, that is transformed into a user-facing exception Error.unexpected_empty_element in the main module.)

If you do this, and if the user respects the precondition that the input is local, then the function is fully safe. The only source of memory-unsafety that remains is an in-flight concurrent update (which implies non-locality) that would write a dummy in the array after you have done the check.

[OlivierNicole]

If you do this, and if the user respects the precondition that the input is local, then the function is fully safe.

If the notion of local you use comprises non-escaping (as the Jane Street one does), then I agree.

I disagree on this point, however:

I don't see a good reason to do this, you should check that none of the elements are dummies and fail with Invalid_argument if you find one.

Under the hypothesis that f’s argument is local, no concurrent updates should occur and the dummy elements of the array should be the ones beyond length. So comparing length with the capacity Array.length arr is cheaper and no less safe.

Independent remark: rather than failing when we may be returning dummies, I prefer making a copy for the reason @dbuenzli gave above (allowing users to speculate on the capacity).

[OlivierNicole]

Actually I’m wrong and you’re right: f’s argument can be local and updated concurrently inside f. So the dynarray may be in an invalid state after f returns, and scanning the array removes this case of memory unsafety.

It’s a bit of a pity to pay for an extra array scanning, though.

The choice is between two options:

• We don’t perform a check, and if the function is used in a way that is documented as incorrect, the user gets what the label says (everything may crash and burn).
• We perform the check, which removes a way for the user to shoot themselves in the foot, at the cost of some performance. (But there are other footguns.)

Since the function is marked unsafe, I’m not sure we should spend extra cycles removing some of the many memory corruption cases.

[gasche]

I strongly believe that we should do the check. It is relatively cheap: we want to avoid data copies, not traversals.

None of the ('a -> 'b) -> 'b functions provided in the standard library can statically enforce that their 'a argument is used only locally, even though they document this restriction. But they remain memory-safe even when 'a is leaked, and I strongly believe that we should preserve this property here. The fact that the function is unsafe_ for another reason is not an argument to make it more unsafe than it needs to be.

With the check we get that the function that is fully safe for non-concurrent usage. We can even hope to make it fully safe for concurrent usage in the future if we implement efficient atomic version numbers to detect concurrent races reliably.

[OlivierNicole]

I don’t strongly object.

But just to make sure we are on the same page:

But they remain memory-safe even when 'a is leaked, and I strongly believe that we should preserve this property here.

The current version is already memory-safe when 'a is leaked (and never updated concurrently).

With the check we get that the function that is fully safe for non-concurrent usage.

It is already the case.

[gasche]

The current version is safe whenever the function ('a t -> unit) returns with 'a t being a well-formed dynarray -- it does not contain a dummy up to its specified length. Concurrent races are one way to create an ill-formed dynarray, but I would not be fully confident that it is the only one -- this relies on global reasoning on the whole Dynarray implementation that is complex and fragile and probably does not hold today.

On the other hand, with a dynamic check that there are no dummies (as we do on get and on to_list, to_array, etc.), we can ensure memory-safety via simple, local reasoning.

[gasche]

Okay, by eyeballing the implementation, here is one situation in which you can create an ill-formed dynarray in a sequential program. (Note that this should not break soundness, as the implementation is defensive against ill-formed arrays.)

• In the implementation of Dynarray.append_array, the length of the dynarray is updated before we call Dummy.Array.blit_array to blit the elements coming from the array argument.
• Dummy.Array.blit_array will allocate boxed floats if the array argument is a flat float array
• These allocations can fail with an asynchronous exception (Out_of_memory or an exception coming from some GC callback, finalizer, Control-C, etc.) and interrupt append_array, leaving the dynarray in an ill-formed state.

[OlivierNicole]

I see. Asynchronous exceptions are hard…

I have updated the implementation to check for the absence of dummies.

[gasche]

I like the implementation now! I moved to some style comments on the documentation, and asking for a couple more tests. I am approving in principle -- I think that these issues will be resolved in some way or some other before we get our required second maintainer approval for stdlib changes.

[gasche]

Two different CI machines (linux-arm64, clang-cl 64 bits) are failing on the test weak-ephe-final/ephe_custom.opt:

> Fatal error: exception Invalid_argument("option is None")

I'm certain this is unrelated to the current PR, but this is more than the occasional flakiness.

[dbuenzli]

Would it be possible to have that in 5.4 ? It's a bit annoying when features are introduced over multiple OCaml versions.

[gasche]

This PR was in an almost-merged state two weeks ago, but we forgot about it and now the release branch has already been created by our release manager, @Octachron, so I suspect that it is too late. The PR still requires an approval from a second maintainer, per stdlib standards, so it's not ready to be merged.

[gasche]

We need to fix the Changes conflict here, and then I propose to merge in trunk.

(To be backported in 5.4 or not based on Florian's gut feeling.)

[OlivierNicole]

I rebased on trunk, the Windows CI refuses to build for some strange reason.

[Octachron]

Ok, looking at the feature and the implementation this seems reasonable in term of complexity (compared to my expectation for zero-copy implementation in a concurrent settings).

I am approving for 5.4 .

[Octachron]

Cherry-picked on 5.4 as 0fc7906 .