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

caml_make_array assumes small arrays #6460

Closed
vicuna opened this issue Jun 17, 2014 · 3 comments
Closed

caml_make_array assumes small arrays #6460

vicuna opened this issue Jun 17, 2014 · 3 comments

Comments

@vicuna
Copy link

@vicuna vicuna commented Jun 17, 2014

Original bug ID: 6460
Reporter: @lpw25
Assigned to: @damiendoligez
Status: closed (set by @xavierleroy on 2016-12-07T10:34:20Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.0+dev
Target version: 4.02.0+dev
Fixed in version: 4.02.0+dev
Category: runtime system and C interface
Tags: patch, junior_job
Monitored by: @gasche @jmeber @hcarty

Bug description

caml_make_array asserts that the size of the array is less than Max_young_wosize so that it can use caml_alloc_small. However, the compiler does not enforce this invariant.

$ cat test.ml
let f x =
  [| x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
     x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; |]
 
let _ = f 3.5

$ ocamlopt -runtime-variant d test.ml

$ ./a.out 
### OCaml runtime: debug mode ###
Initial minor heap size: 2048k bytes
Initial major heap size: 3840k bytes
Initial space overhead: 80%
Initial max overhead: 500%
Initial heap increment: 15%
Initial allocation policy: 0
file array.d.c; line 217 ### Assertion failed: size < Max_young_wosize

File attachments

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 20, 2014

Comment author: nevor

This bug is interesting because any literal given (including the one in test.ml) will be valid as long as it is smaller than the size of the minor heap which is a lot bigger than the enforced invariant.

So we found a minimal test that trashes the memory because of the violated invariant and we have attached a simple patch that corrects the problem by calling caml_alloc_shr when the size goes over Max_young_wosize. The new example and the patch work both on bytecode and native.

Maybe this was not done before for a good reason, in that case we might consider something along the lines of refusing too big literals, any constant smaller than Minor_heap_min would be good enough.

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 17, 2014

Comment author: @damiendoligez

Patch will be applied before 4.02.0.

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 18, 2014

Comment author: @xavierleroy

Fixed as proposed in 4.02 (commit 15010) and on trunk (commit 15011).

I heard some questions about the use of caml_check_urgent_gc after caml_alloc_shr. This is correct, this is cheap, and this can help memory performance. caml_alloc_shr never triggers a GC, because often it is being called during a minor GC. However, caml_alloc_shr can set flags saying that it is "urgent" to perform a GC in the future. caml_check_urgent_gc will check and honor this flag, without waiting for the next polling of this flag by the bytecode interpreter or generated native code.

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

Successfully merging a pull request may close this issue.

None yet
2 participants