Remove use of C variable length arrays (VLAs)

[dra27]

OCaml 5 adds caml_alloc_1..caml_alloc_9 and the general caml_alloc_N to caml/alloc.h (see
ocaml-multicore/ocaml-multicore#72). These are implemented using C99 variable length arrays (VLAs). VLAs are (if I understand correctly) deprecated in C11 and they're not supported in our favourite MS C compiler (and they have no intention of adding them as part of their C11 and C17 support).

This PR is intended mainly to start discussion - at this stage I've removed caml_alloc_N, but it may be possible to reintroduce it as a macro (cf. CAMLlocalN) if we wanted?

I've reimplemented the 9 allocation functions with the inlined function as a macro, but this needs checking to ensure that the generated code is still correct (cf. the discussion in the original PR and cc @stedolan and @gadmm).

[gasche]

I can see how the current implementation of caml_alloc_N uses a variable-length array (VLA), so you removed it. But I don't understand what is the issue with the implementation of the caml_alloc_[1-9] functions, in b7a5bb5 you replaced a function by a macro, but I don't see any variable-length array in the code before or after the change? (The do_alloc_small function takes a value* array with an explicit size parameter, which seems fine. Enter_gc uses CAMLlocalN and I don't know how it is implemented, but the same Enter_gc code should be used before and after the patch anyway.)

[gadmm]

My understanding is that CAMLlocalN uses a VLA when the size argument is not a compile-time constant. By writing do_alloc_small as a macro, we get the constant value in time for avoiding a VLA in CAMLlocalN.

CAMLlocalN is part of the public API so it should itself be fixed, so that it never uses a VLA. How about replacing both uses of VLAs in CAMLlocalN and caml_alloc_N with a call to alloca/_malloca?

(In a second time we can look at the generated code to see if it generates better code after the transformation into a macro, but this is a priori not needed for correctness.)

[dra27]

It's the use of CAMLlocalN with the wosize argument which creates the VLA. Converting do_alloc_small to a macro means that the constant gets threaded through instead. The configure change proposed here allows users to continue using CAMLlocalN to create VLAs if they wish, it just prohibits our code from doing it.

[gasche]

The previous caml_alloc_[1-9] functions already had a hairy definition (with a tricky local copy for rooting etc.), and the new version is slightly worse (two macros that depend on each other, etc.). I wonder if we should consider biting the bullet and copy-pasting the definition 9 times. This would also directly avoid CAMLlocalN.

[smuenzel]

CAMLlocalN is part of the public API so it should itself be fixed, so that it never uses a VLA. How about replacing both uses of VLAs in CAMLlocalN and caml_alloc_N with a call to alloca/_malloca?

I tried doing this here: trunk...smuenzel:ocaml:no-vla
I think that should pretty much be equivalent, if I'm not missing something

[dra27]

It does indeed look equivalent, thanks @smuenzel (I think it needs a slight tweak to use _alloca on MSVC, but that's not relevant at the moment). I'm not sure this has to be fixed though - CAMLlocalN doesn't always use a VLA, it only does that if you pass a non-constant argument to it, versus the other functions added which always were using VLAs. Given that alloca is - strangely - not POSIX, it's not totally clear to me that we have to fix it.

I haven't checked, but given that caml_alloc_N is an OCaml 5.0, it's not an impossibility that we simply choose to remove it, rather than re-implement it? Or, alternately, reimplement it as a macro (as with e.g. Alloc_small, taking the result as the first argument instead of it being an inlined function).

[xavierleroy]

alloca is much worse than variable-length arrays, in my opinion. At least VLAs are specified in a standard... No alloca shall enter the OCaml runtime system!

I didn't notice that CAMLlocalN creates a VLA if the number N is not a compile-time constant, but so be it. In the runtime system I see only one use (in otherlibs/runtime_events/runtime_events_consumer.c) with N = 4.

I'm in favor of removing caml_alloc_N. I think caml_alloc_1 to _9 cover all reasonable cases where we want to pass the values of the fields as argument to a function (as opposed to e.g. filling them later by copying from an array or what not).

[dra27]

We're still just at the point with 5.0 release cycle where caml_alloc_N can be removed - should I open a separate PR just removing caml_alloc_N from 5.0 (and leaving the VLA implementation of caml_alloc_1, etc. to be removed here in 5.1) or shall we add this PR to the 5.0 milestone?

[gasche]

should I open a separate PR just removing caml_alloc_N from 5.0

This sounds like a good idea to me.

[gadmm]

Following the discussion, this looks essentially good. Both this approach with a macro and the alternative suggested by Garbiel (inlining DO_ALLOC_SMALL) sounds equally good to me.

[gadmm]

This description needs to be updated now that caml_alloc_N never made it into OCaml 5 (esp. no breaking change).

[gadmm]

Nit-pick. I'd write Do_alloc_small and use parentheses around the occurrences of the arguments, e.g. (tag) < 256.

Can you please add one comment line here to explain to future readers why you had to use a macro?

[gadmm]

@gasche This looks ready. I am also convinced now that it is better to have a macro than to duplicate code.

(Please credit other reviewers who helped come up with the best version.)

[dra27]

Actually, it's not quite there - the name vals is fixed (it has to be, for the Enter_gc macro) but it's passed to Do_alloc_small as though it could be altered. I'll push a correction for that (with more comment) shortly. I also found that trying to expand the macros into 9 separate functions gets even messier.

[gasche]

the name vals is fixed (it has to be, for the Enter_gc macro)

Naming suggestion for that version of the macro: Enter_GC_saving_vals.

Edit: or maybe: Copy_vals_and_enter_GC.

[dra27]

I went with "preserve" - so Enter_gc_preserve_vals - how's the final result looking?

[gasche]

Actually, could this be just

CAMLexport value caml_alloc_3 (tag_t tag, value a, value b, value c)
{
  Do_alloc_small(3, tag, {a, b, c});
}

?

With this form we can bind vals in Do_alloc_small itself, and thus reduce the scope of the code that has to reason about vals as a magical/fixed name.

[dra27]

I don't think it can work - the { ... } is an initialiser syntax and it doesn't work as a bracket for the macro (that gets seen by the preprocessor as a 5-argument macro call)

[xavierleroy]

Just add parentheses! Do_alloc_small(3, tag, ({a, b, c})) should pass the preprocessor. But then you could just omit the braces and add them back in the macro.

[dra27]

I'd tried that - but then you end up with value vals[3] = ({a, b, c}) which is a syntax error. I hadn't considered passing them as (a, b, c) and then restoring the braces in the macro, but that doesn't appear to work either ("left-hand operand of comma expression has no effect" presumably arising from `value vals[3] = {(a, b, c)}).

It does "work" with __VA_ARGS__, - what do you think??

[gasche]

It looks like a sophisticated monster born out of the best intentions, I love it. Approved.