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 wrong register typing in float (and boxed int) unboxing #2083

Merged
merged 9 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@trefis
Copy link
Contributor

commented Oct 1, 2018

$ cat unbox_under_switch.ml
type _ t =
  | IO : int option t
  | F : float t

let bar : type a. a t -> float -> int -> a =
  fun t f i ->
    match t with
    | IO -> Some i
    | F -> f
[@@inline always]

let foo (t : float t) f i =
  let r = ref 0. in
  r := bar t f i

$ ocamlopt -c ./unbox_under_switch.ml
Fatal error: exception File "asmcomp/cmm.ml", line 83, characters 4-10: Assertion failed
Raised at file "asmcomp/cmm.ml", line 83, characters 4-16
Called from file "asmcomp/selectgen.ml", line 144, characters 13-51
Called from file "asmcomp/selectgen.ml", line 769, characters 18-42
Called from file "asmcomp/selectgen.ml", line 671, characters 18-32
Called from file "asmcomp/selectgen.ml", line 1102, characters 18-32
Called from file "asmcomp/selectgen.ml", line 1207, characters 2-16
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "asmcomp/asmgen.ml" (inlined), line 102, characters 15-18
Called from file "asmcomp/asmgen.ml", line 107, characters 2-75
Called from file "list.ml", line 106, characters 12-15
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "asmcomp/asmgen.ml" (inlined), line 102, characters 15-18
Called from file "asmcomp/asmgen.ml", line 180, characters 2-122
Called from file "asmcomp/asmgen.ml", line 155, characters 6-12
Re-raised at file "asmcomp/asmgen.ml", line 160, characters 6-15
Re-raised at file "asmcomp/asmgen.ml", line 171, characters 4-13
Called from file "driver/optcompile.ml" (inlined), line 67, characters 15-18
Called from file "driver/optcompile.ml", line 127, characters 16-303
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "driver/optcompile.ml" (inlined), line 67, characters 15-18
Called from file "driver/optcompile.ml", line 121, characters 10-626
Called from file "driver/optcompile.ml", line 139, characters 8-68
Re-raised at file "driver/optcompile.ml", line 144, characters 6-13
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "driver/compenv.ml", line 578, characters 6-35
Called from file "list.ml", line 106, characters 12-15
Called from file "driver/compenv.ml", line 654, characters 2-61
Called from file "driver/optmain.ml", line 256, characters 6-163
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Called from file "parsing/location.ml" (inlined), line 499, characters 31-61
Called from file "driver/optmain.ml", line 316, characters 6-37
Called from file "driver/optmain.ml", line 320, characters 2-9

This was initially noticed when building Owl with -inline 20, which caused that call to Ctypes.(!@) to be inlined.

The bug is also present (and was noticed) on 4.07.

The fix was suggested by @lpw25.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

I'd be in favor of backporting to the 4.07 branch and make an rc2.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Suggested by @lpw25 and yours truly.

The patch looks correct to me.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

Suggested by @lpw25 and yours truly.

Apologies :)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

check_typo was unhappy, I pushed a fix as well as a changelog entry; all the other tests passed.
Whoever approves should feel free to squash and merge.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

I see the same header-ignoring logic in the third case of the unbox_int function. Should it be fixed as well?

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

Well spotted. I think that code is wrong as well but won't actually cause a failure at present (because the types of registers used to hold unboxed integers and arbitrary values are compatible with respect to "join" in Selectgen). However some work exists (#1192) which tightens up the register typing specifically with a view to catching this kind of error. (I would still appreciate feedback on that work.) If such change to the typing were merged, then the code in unbox_int could cause a compile-time failure, because the type of a register holding an unboxed integer (which cannot be scanned by the GC) would be incompatible with the type of a register holding an arbitrary value (corresponding to the Some case in the test case above). So we should fix unbox_int, I think.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

I've pushed a fix for unbox_int. @lpw25 has reviewed this diff (591718e).

@mshinwell mshinwell changed the title Fix excessively aggressive float unboxing Fix excessively aggressive float (and boxed int) unboxing Oct 2, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@chambart Could you please look at this?

@mshinwell mshinwell changed the title Fix excessively aggressive float (and boxed int) unboxing Fix wrong register typing in float (and boxed int) unboxing Oct 2, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@damiendoligez Do you want to backport this to 4.07? It is needed to avoid a failure when compiling Owl, so I understand.

@chambart

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

Looks good to me. You could be a bit more aggressive in that case: It would be sensible to produce some kind of trap (like the Uunreachable case).

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

@chambart Agreed. However, I think that would involve adding cases that explicitly match the ill-typed cases, in addition to the well-typed cases that are picked out at the moment. I'm not sure if it's worth doing this. In the back of my mind I have that the next version of Flambda should have eliminated these branches (or replaced them with traps) by the time we get here. How strongly do you feel about this?

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

@chambart Ping (we should make sure this goes into 4.08)

@mshinwell mshinwell added this to the 4.08 milestone Oct 16, 2018

@mshinwell mshinwell added the bug label Oct 16, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Note: unless @chambart speaks up, I will rebase and merge this on Tuesday.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@trefis Pierre is away for a while. I think it's ok to merge this in its current form, and if needs be it can be improved upon later.

trefis and others added some commits Oct 1, 2018

adds a test showing incorrect unboxing
$ ocamlrun ocamlopt -I %{root}/runtime -nostdlib -I %{root}/stdlib -o unbox_under_switch.opt unbox_under_switch.ml

Fatal error: exception File "asmcomp/cmm.ml", line 83, characters 4-10: Assertion failed
Raised at file "asmcomp/cmm.ml", line 83, characters 4-16
Called from file "asmcomp/selectgen.ml", line 152, characters 13-51
Called from file "asmcomp/selectgen.ml", line 777, characters 18-42
Called from file "asmcomp/selectgen.ml", line 679, characters 18-39
Called from file "asmcomp/selectgen.ml", line 1108, characters 18-39
Called from file "asmcomp/selectgen.ml", line 1214, characters 2-35
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "asmcomp/asmgen.ml", line 106, characters 2-75
Called from file "list.ml", line 107, characters 12-15
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "asmcomp/asmgen.ml", line 176, characters 2-140
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "asmcomp/asmgen.ml", line 155, characters 7-231
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "driver/optcompile.ml", line 68, characters 7-198
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "driver/compile_common.ml", line 123, characters 6-68
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "utils/misc.ml", line 31, characters 8-15
Re-raised at file "utils/misc.ml", line 45, characters 40-48
Called from file "driver/compenv.ml", line 588, characters 6-57
Called from file "list.ml", line 107, characters 12-15
Called from file "driver/compenv.ml", line 664, characters 2-61
Called from file "driver/optmain.ml", line 270, characters 6-163
Re-raised at file "parsing/location.ml", line 633, characters 22-25
Called from file "driver/optmain.ml", line 336, characters 6-37
Called from file "driver/optmain.ml", line 340, characters 2-9

@trefis trefis force-pushed the trefis:unbox_float branch from 6c5f43b to fe4c3da Nov 5, 2018

@lpw25

lpw25 approved these changes Nov 5, 2018

@trefis trefis merged commit d85e98a into ocaml:trunk Nov 5, 2018

2 checks passed

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

@trefis trefis deleted the trefis:unbox_float branch Nov 5, 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.