search

rdicosmo / parmap

Parmap is a minimalistic library allowing to exploit multicore architecture for OCaml programs with minimal modifications.
http://rdicosmo.github.io/parmap/
Other
94 stars 20 forks source link

Use Array.create_float instead of Obj, dropping support for ocaml 4.02.3 #100

Closed jamesjer closed 3 years ago

jamesjer commented 3 years ago

I am working on packaging parmap for the Fedora Linux distribution. An initial build attempt failed on the 32-bit builders, as noted in https://github.com/rdicosmo/parmap/issues/92. Retrying with one zero removed from 10000000 in both floatscale.ml and simplescale.ml caused the simplescale test to succeed, but the floatscale test segfaulted, as also noted in https://github.com/rdicosmo/parmap/issues/92.

A run of the floatscale test under valgrind shows that an out-of-bounds array access is the cause:

==37== Invalid write of size 4
==37==    at 0x48457C4: memcpy (vg_replace_strmem.c:1051)
==37==    by 0x15B921: UnknownInlinedFun (string_fortified.h:29)
==37==    by 0x15B921: ml_blit_bigarray_to_floatarray (bytearray_stubs.c:71)
==37==    by 0x15D258: camlBytearray__to_floatarray_450 (bytearray.ml:117)
==37==    by 0x15C6C6: camlUtils__array_float_scale_test_inner_460 (utils.ml:89)
==37==    by 0x15B6E4: camlFloatscale__entry (floatscale.ml:62)
==37==    by 0x15812B: caml_program (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C7D98: caml_start_program (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C8103: caml_startup_common (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C81BD: caml_main (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x157D98: main (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==  Address 0x4d57c68 is 0 bytes after a block of size 249,880 alloc'd
==37==    at 0x483E6FE: malloc (vg_replace_malloc.c:380)
==37==    by 0x1AAD8B: caml_stat_alloc_noexc (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1AB137: caml_alloc_for_heap (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C01C1: caml_compact_heap (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1BB7E3: caml_gc_compaction (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x15D8EE: camlParmap__spawn_many_752 (parmap.ml:177)
==37==    by 0x15DC95: camlParmap__simplemapper_872 (parmap.ml:190)
==37==    by 0x15BCBD: camlUtils__scale_test_inner_396 (utils.ml:29)
==37==    by 0x15B5B0: camlFloatscale__entry (floatscale.ml:56)
==37==    by 0x15812B: caml_program (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C7D98: caml_start_program (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)
==37==    by 0x1C8103: caml_startup_common (in /builddir/build/BUILD/parmap-1.2/_build/default/tests/floatscale.exe)

The memory block is allocated inside of GC_compact(), suggesting that something was moved, but the code has a pointer that was not updated. The pointer that is accessed is created in bytearray.ml, here:

let to_floatarray a l =
  let fa = Obj.obj (Obj.new_block Obj.double_array_tag l) in
  unsafe_blit_to_floatarray a 0 fa 0 l;
  fa

With the change in this patch, using Array.create_float instead of Obj, the segfaults no longer occur. Unfortunately, that means dropping support for OCaml 4.02.3.

UnixJunkie commented 3 years ago

Can you also send your patch there also: https://github.com/UnixJunkie/bytearray bytearray was factored out of parmap, for reuse by other libraries

rdicosmo commented 3 years ago

Thanks a lot, @jamesjer, for identifying this issue, that was in #92 too. It would be interesting to know exactly what changed in recent versions of the OCaml compiler that led to this behaviour, but that's not a blocker for moving forward. I do not mind dropping support for OCaml 4.02.3. I'm merging this and will make a new release soon.

rdicosmo commented 3 years ago

New release 1.2.1 done (and published to opam-repository). Thanks again @jamesjer