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

Fix caml_make_float_vect #2183

Merged
merged 6 commits into from Dec 28, 2018

Conversation

Projects
None yet
4 participants
@damiendoligez
Copy link
Member

commented Dec 6, 2018

This PR fixes bugs revealed by the test file of #1936.

When OCaml is configured with -no-flat-float-array, the Array.create_float primitive is completely wrong:

Before this patch:

ocaml
        OCaml version 4.08.0+dev0-2018-04-09

# Array.create_float (-1);;
Out of memory during evaluation.
# Array.create_float 3;;
Segmentation fault: 11

After this patch:

ocaml
        OCaml version 4.08.0+dev0-2018-04-09

# Array.create_float (-1);;
Exception: Invalid_argument "Array.create_float".
# Array.create_float 3;;
- : float array = [|0.; 0.; 0.|]

cc @gasche

@damiendoligez damiendoligez added the bug label Dec 6, 2018

@damiendoligez damiendoligez added this to the 4.08 milestone Dec 6, 2018

if (wosize > Max_wosize) caml_invalid_argument ("Array.create_float");
result = caml_alloc (Long_val (len), 0);
for (i = 0; i < wosize; i++){
x = caml_alloc_small (Double_wosize, Double_tag);

This comment has been minimized.

Copy link
@yallop

yallop Dec 6, 2018

Member

If I'm not mistaken, this function could be made quite a bit faster by moving this allocation out of the loop (i.e. by initializing every element of the array with the same value).

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 6, 2018

Author Member

Yes, I realized that on my way home... I'll change this PR soon.

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Dec 6, 2018

@gasche
Copy link
Member

left a comment

I'm a bit worried by the amount of code duplication in the array.c file. Why cannot we share more logic, with the generic version calling either the floatarray functions or the float_array functions depending on whether FLAT_FLOAT_ARRAY is set?

Show resolved Hide resolved runtime/array.c Outdated
@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2018

Why cannot we share more logic

This should probably be done in another PR.

@gasche

gasche approved these changes Dec 8, 2018

Copy link
Member

left a comment

I have read the new implementation and believe that it is correct and efficient. Do we have tests?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2018

Show resolved Hide resolved runtime/array.c Outdated
Show resolved Hide resolved runtime/array.c Outdated

damiendoligez added some commits Dec 6, 2018

When configured with -no-flat-float-array, the primitive caml_make_fl…
…oat_vect

(known as Array.create_float in OCaml) is completely broken:
1. it fails to raise Invalid_argument when the size is negative or too large.
2. it fails to initialize its result, which immediately leads to segfault.
In -no-flat-float-array mode, allocate the uninitialized float value …
…(used

for Array.create_float) only once and reuse it for all new "uninitialized"
arrays.

@damiendoligez damiendoligez force-pushed the damiendoligez:fix-array-create-float branch from 47a4a9f to c040466 Dec 21, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

Thanks @gasche and @yallop. I'll merge after CI passes.

@damiendoligez damiendoligez merged commit c244e2d into ocaml:trunk Dec 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@damiendoligez damiendoligez deleted the damiendoligez:fix-array-create-float branch Dec 28, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.