mirage / bechamel

Agnostic benchmark in OCaml (proof-of-concept)
MIT License
44 stars 15 forks source link

Bechamel: fix unsafe off by one causing SIGSEGV on OCaml 5.0 #43

Closed edwintorok closed 1 year ago

edwintorok commented 1 year ago

My benchmark was running fine on OCaml 4.x but always segfaulting on OCaml 5.0. Took a while to track down the root cause, was trying to eliminate all unsafe code one by one until I found the problem: replacing the unsafe_array_get with Array.get raised an out of bounds error, which immediately highlighted the off by one error.

(I think some of the unsafe code usage could actually be replaced with something safer without sacrificing performance, I'll open another PR if I have something useful).

This change makes my benchmarks able to run on OCaml 5 again!

edwintorok commented 1 year ago

There are more places where if I replace the unsafe accesses with Array.get/set I get out of bounds, e.g.:

Fatal error: exception Invalid_argument("index out of bounds")
Raised by primitive operation at Bechamel__Unsafe.unsafe_array_get in file "lib/xapi-stdext-encodings/bechamel/lib/unsafe.ml", line 6, characters 23-32
Called from Bechamel__Benchmark.run in file "lib/xapi-stdext-encodings/bechamel/lib/benchmark.ml", line 186, characters 25-53
Called from Bechamel__Benchmark.all in file "lib/xapi-stdext-encodings/bechamel/lib/benchmark.ml", line 237, characters 18-44
Called from Dune__exe__Fact.benchmark in file "lib/xapi-stdext-encodings/bechamel/examples/fact.ml", line 66, characters 4-105
Called from Dune__exe__Fact in file "lib/xapi-stdext-encodings/bechamel/examples/fact.ml", line 84, characters 16-28
edwintorok commented 1 year ago

This is because Uniq.run allocates an empty array, so fetching item at index 0 is out of bounds... (which explains why Uniq was segfaulting too, before I switched to using Multiple)

edwintorok commented 1 year ago

Fixed the segfault in Uniq too, it should now be possible to use bechamel without segfaults...

dinosaure commented 1 year ago

Thanks! I will cut a release as soon as I can.