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 fatal error when compiling objects with Flambda #965

Merged
merged 6 commits into from Feb 15, 2017

Conversation

Projects
None yet
6 participants
@mshinwell
Contributor

mshinwell commented Dec 12, 2016

(Was PR#7426.)

Translclass uses array operations on object blocks because there is no primitive to set the field of a block using a computed index. At present these "arrays" are said to have kind Paddrarray which causes an error in the Flambda simplifier in the event that a float is assigned to them. I think this error is correct. I believe the reason is as follows:

  • There is an invariant that if an array contains a float then it must have tag Double_array_tag. (Since we're dealing with arrays not blocks, all elements have the same type.)
  • It follows that there is no concept in the middle- and back end of an array that contains floats but where such floats are boxed. As such, it is illegal to use Paddrarray operations with float operands.

One solution might be to use Pgenarray instead, but this might penalise code using objects; also, it's still a lie to the optimizers (which we should particularly avoid now that Flambda exists). Instead I've done what @garrigue suggested and introduced two new primitives for getting and setting fields of blocks where the offset is not necessarily known at compile time. I have not yet added code to the Flambda simplifier to handle the new Pfield_computed like the existing Pfield when the offset turns out to be known; this can be done later.

@mshinwell mshinwell added the bug label Dec 12, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 12, 2016

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Dec 13, 2016

Contributor

I was a bit afraid that adding 2 instructions to lambda would be heavy, but actually it's really light weight.
I suppose this is the way to go.

Contributor

garrigue commented Dec 13, 2016

I was a bit afraid that adding 2 instructions to lambda would be heavy, but actually it's really light weight.
I suppose this is the way to go.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 27, 2016

Contributor

@chambart Please review if you could, thanks.

Contributor

mshinwell commented Dec 27, 2016

@chambart Please review if you could, thanks.

@chambart

This comment has been minimized.

Show comment
Hide comment
@chambart

chambart Jan 18, 2017

Contributor

Ok, this looks good is cleaner than using Popaque and might be useful elsewhere.

Contributor

chambart commented Jan 18, 2017

Ok, this looks good is cleaner than using Popaque and might be useful elsewhere.

@chambart chambart added the approved label Jan 18, 2017

mshinwell added some commits Jan 18, 2017

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 19, 2017

Contributor

@chambart Please re-review the case in cmmgen.ml for Psetfield_computed, I had to rebase this across the change to the initialization_or_assignment type. You can add the "approved" label again once that's done.

Contributor

mshinwell commented Jan 19, 2017

@chambart Please re-review the case in cmmgen.ml for Psetfield_computed, I had to rebase this across the change to the initialization_or_assignment type. You can add the "approved" label again once that's done.

@mshinwell mshinwell removed the approved label Jan 19, 2017

Show outdated Hide outdated asmcomp/cmmgen.ml
| (Assignment | Heap_initialization), Pointer ->
return_unit (
addr_array_set (transl env arg1) (transl env arg2) (transl env arg3)
dbg)

This comment has been minimized.

@chambart

chambart Jan 25, 2017

Contributor

Is there a reason not to use caml_initialize instead of caml_modify in case of initialization with a pointer value ?
If this is an error, to avoid potential future problems, I also suggest sharing the logic by something like:

type assignment_kind = Caml_modify | Caml_initialize | Simple

let assignment_kind ptr init =
  match init, ptr with
  | Assignment, Pointer -> Caml_modify
  | Heap_initialization, Pointer -> Caml_initialize
  | Assignment, Immediate
  | Heap_initialization, Immediate
  | Root_initialization, (Immediate | Pointer) -> Simple
@chambart

chambart Jan 25, 2017

Contributor

Is there a reason not to use caml_initialize instead of caml_modify in case of initialization with a pointer value ?
If this is an error, to avoid potential future problems, I also suggest sharing the logic by something like:

type assignment_kind = Caml_modify | Caml_initialize | Simple

let assignment_kind ptr init =
  match init, ptr with
  | Assignment, Pointer -> Caml_modify
  | Heap_initialization, Pointer -> Caml_initialize
  | Assignment, Immediate
  | Heap_initialization, Immediate
  | Root_initialization, (Immediate | Pointer) -> Simple
@chambart

This comment has been minimized.

Show comment
Hide comment
@chambart

chambart Feb 15, 2017

Contributor

Rereviewed. This is good to go.

Contributor

chambart commented Feb 15, 2017

Rereviewed. This is good to go.

@chambart chambart added the approved label Feb 15, 2017

@mshinwell mshinwell merged commit 2711420 into ocaml:trunk Feb 15, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@toots toots referenced this pull request Sep 2, 2017

Merged

Liquidsoap 1.3.2 #10207

@drvink

This comment has been minimized.

Show comment
Hide comment
@drvink

drvink Oct 7, 2017

I don't think this issue is fully fixed, as I am getting this error when trying to build core_kernel.v0.9.115.18+07 on the 4.05.0+flambda switch (Linux x64).

This should repro:

opam init --compiler=4.05.0+flambda
opam repo -k git add janestreet https://github.com/janestreet/opam-repository.git
opam install core_kernel
[ERROR] The compilation of core_kernel failed at "jbuilder build -p core_kernel -j 4".

#=== ERROR while installing core_kernel.v0.9.115.18+07 ========================#
# opam-version 1.2.2
# os           linux
# command      jbuilder build -p core_kernel -j 4
# path         /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07
# compiler     4.05.0+flambda
# exit-code    1
# env-file     /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.env
# stdout-file  /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.out
# stderr-file  /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.err
### stderr ###
# [...]
#     ocamlopt src/core_kernel__Float.{cmx,o}
#     ocamlopt src/core_kernel__Force_once.{cmx,o}
#     ocamlopt src/core_kernel__Obj_array.{cmx,o} (exit 2)
# (cd _build/default && /home/mdl/.opam/4.05.0+flambda/bin/ocamlopt.opt -w -40 -g -I /home/mdl/.opam/4.05.0+flambda/lib/base -I /home/mdl/.opam/4.05.0+flambda/lib/base/caml -I /home/mdl/.opam/4.05.0+flambda/lib/base/md5 -I /home/mdl/.opam/4.05.0+flambda/lib/base/shadow_stdlib -I /home/mdl/.opam/4.05.0+flambda/lib/bin_prot -I /home/mdl/.opam/4.05.0+flambda/lib/bin_prot/shape -I /home/mdl/.opam/4.05.0+flambda/lib/fieldslib -I /home/mdl/.opam/4.05.0+flambda/lib/jane-street-headers -I /home/mdl/.opam/4.05.0+flambda/lib/num -I /home/mdl/.opam/4.05.0+flambda/lib/ocaml -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_assert/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_bench/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_compare/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/collector -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/common -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/config -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_hash/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_inline_test/config -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_inline_test/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/sexplib -I /home/mdl/.opam/4.05.0+flambda/lib/stdio -I /home/mdl/.opam/4.05.0+flambda/lib/typerep -I /home/mdl/.opam/4.05.0+flambda/lib/variantslib -I base_for_tests/src -no-alias-deps -I src -open Core_kernel__ -o src/core_kernel__Obj_array.cmx -c -impl src/obj_array.pp.ml)
# >> Fatal error: Assignment of a float to a specialised non-float array: (array.unsafe_set[int]<{src/obj_array.ml:185,10-32;src/obj_array.ml:93,2-20;src/obj_array.ml:79,7-68;src/obj_array.ml:56,2-56}>
#                                                           t/1960 i/1963
#                                                           obj/1954)
# Fatal error: exception Misc.Fatal_error
#
# Waiting for 1 job to finish.

drvink commented Oct 7, 2017

I don't think this issue is fully fixed, as I am getting this error when trying to build core_kernel.v0.9.115.18+07 on the 4.05.0+flambda switch (Linux x64).

This should repro:

opam init --compiler=4.05.0+flambda
opam repo -k git add janestreet https://github.com/janestreet/opam-repository.git
opam install core_kernel
[ERROR] The compilation of core_kernel failed at "jbuilder build -p core_kernel -j 4".

#=== ERROR while installing core_kernel.v0.9.115.18+07 ========================#
# opam-version 1.2.2
# os           linux
# command      jbuilder build -p core_kernel -j 4
# path         /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07
# compiler     4.05.0+flambda
# exit-code    1
# env-file     /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.env
# stdout-file  /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.out
# stderr-file  /home/mdl/.opam/4.05.0+flambda/build/core_kernel.v0.9.115.18+07/core_kernel-93610-e4db2f.err
### stderr ###
# [...]
#     ocamlopt src/core_kernel__Float.{cmx,o}
#     ocamlopt src/core_kernel__Force_once.{cmx,o}
#     ocamlopt src/core_kernel__Obj_array.{cmx,o} (exit 2)
# (cd _build/default && /home/mdl/.opam/4.05.0+flambda/bin/ocamlopt.opt -w -40 -g -I /home/mdl/.opam/4.05.0+flambda/lib/base -I /home/mdl/.opam/4.05.0+flambda/lib/base/caml -I /home/mdl/.opam/4.05.0+flambda/lib/base/md5 -I /home/mdl/.opam/4.05.0+flambda/lib/base/shadow_stdlib -I /home/mdl/.opam/4.05.0+flambda/lib/bin_prot -I /home/mdl/.opam/4.05.0+flambda/lib/bin_prot/shape -I /home/mdl/.opam/4.05.0+flambda/lib/fieldslib -I /home/mdl/.opam/4.05.0+flambda/lib/jane-street-headers -I /home/mdl/.opam/4.05.0+flambda/lib/num -I /home/mdl/.opam/4.05.0+flambda/lib/ocaml -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_assert/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_bench/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_compare/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/collector -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/common -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_expect/config -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_hash/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_inline_test/config -I /home/mdl/.opam/4.05.0+flambda/lib/ppx_inline_test/runtime-lib -I /home/mdl/.opam/4.05.0+flambda/lib/sexplib -I /home/mdl/.opam/4.05.0+flambda/lib/stdio -I /home/mdl/.opam/4.05.0+flambda/lib/typerep -I /home/mdl/.opam/4.05.0+flambda/lib/variantslib -I base_for_tests/src -no-alias-deps -I src -open Core_kernel__ -o src/core_kernel__Obj_array.cmx -c -impl src/obj_array.pp.ml)
# >> Fatal error: Assignment of a float to a specialised non-float array: (array.unsafe_set[int]<{src/obj_array.ml:185,10-32;src/obj_array.ml:93,2-20;src/obj_array.ml:79,7-68;src/obj_array.ml:56,2-56}>
#                                                           t/1960 i/1963
#                                                           obj/1954)
# Fatal error: exception Misc.Fatal_error
#
# Waiting for 1 job to finish.
@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 10, 2017

Member

@drvink could you please enter a report at https://caml.inria.fr/mantis, lest we forget about this problem?

Member

damiendoligez commented Oct 10, 2017

@drvink could you please enter a report at https://caml.inria.fr/mantis, lest we forget about this problem?

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Oct 10, 2017

Contributor

The above is not the same bug. It is the issue discussed at #1169. In the end we decided that this was a bug in core_kernel rather than in the compiler -- or rather that the internal invariants of the compiler changed and core_kernel would have to change with them. That change has not yet been made to core_kernel, but it will be made before there are any official releases that support 4.05.0.

Contributor

lpw25 commented Oct 10, 2017

The above is not the same bug. It is the issue discussed at #1169. In the end we decided that this was a bug in core_kernel rather than in the compiler -- or rather that the internal invariants of the compiler changed and core_kernel would have to change with them. That change has not yet been made to core_kernel, but it will be made before there are any official releases that support 4.05.0.

@drvink

This comment has been minimized.

Show comment
Hide comment
@drvink

drvink Oct 10, 2017

@lpw25 That contradicts what was written in https://caml.inria.fr/mantis/view.php?id=7499

If this error is ever seen on user code when using a compiler with the fix for 0007426, even if using unsafe features, another Mantis issue should be opened.

...which referred to this issue on GitHub--my knowledge of OCaml compiler internals is scant, so apologies if this is the ultimately the wrong issue.

That said, I'm happy to write a note on Mantis if you guys simply want a note that references receiving this error message in the context of #965, #1169, and the aforementioned Mantis issue.

drvink commented Oct 10, 2017

@lpw25 That contradicts what was written in https://caml.inria.fr/mantis/view.php?id=7499

If this error is ever seen on user code when using a compiler with the fix for 0007426, even if using unsafe features, another Mantis issue should be opened.

...which referred to this issue on GitHub--my knowledge of OCaml compiler internals is scant, so apologies if this is the ultimately the wrong issue.

That said, I'm happy to write a note on Mantis if you guys simply want a note that references receiving this error message in the context of #965, #1169, and the aforementioned Mantis issue.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Oct 11, 2017

Contributor

I don't really agree with that note: the way it is currently written this fatal error is intended to happen when users are using unsafe features. Really this fatal error should be treated the same as the case of mutating an immutable block -- which is currently handled by a warning. I'm going to talk with Mark and Pierre about these cases because personally I don't even think they should warn since the warnings are fragile and can usually be made to appear in safe code using GADTs.

It might be worth adding a note on Mantis about this case, although the issue was already discussed in the GPR I linked to.

It is worth noting that in this case the compiler has a point, it isn't 100% safe to use that version of Base with 4.05 because it is possible for code to segfault due to the new float array optimisation.

Contributor

lpw25 commented Oct 11, 2017

I don't really agree with that note: the way it is currently written this fatal error is intended to happen when users are using unsafe features. Really this fatal error should be treated the same as the case of mutating an immutable block -- which is currently handled by a warning. I'm going to talk with Mark and Pierre about these cases because personally I don't even think they should warn since the warnings are fragile and can usually be made to appear in safe code using GADTs.

It might be worth adding a note on Mantis about this case, although the issue was already discussed in the GPR I linked to.

It is worth noting that in this case the compiler has a point, it isn't 100% safe to use that version of Base with 4.05 because it is possible for code to segfault due to the new float array optimisation.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment