Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006686OCamlback end (clambda to assembly)public2014-12-01 11:112016-12-07 11:48
Reportershinwell 
Assigned Toshinwell 
PriorityhighSeveritycrashReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version4.02.1 
Target Version4.02.2+dev / +rc1Fixed in Version4.02.2+dev / +rc1 
Summary0006686: subst_boxed_number sometimes fails to record that a boxed value is needed
DescriptionThe test in the following patch fails on 4.02.1, with -inline 2.

>> Fatal error: Selection.emit_expr: unbound var match_1022
Fatal error: exception Misc.Fatal_error

This appears to be happening because subst_boxed_number has two cases (for Cload) that match on patterns involving Cvar nodes---but in the event of them being unable to unbox, they do not execute the logic contained in the normal Cvar case, namely setting [need_boxed] to [true]. This can leave accesses to boxed variables without a corresponding binding of the variable in question.

The patch below also adds an exhaustive match here, since this is quite subtle. There is a new variable ADD_OPTCOMPFLAGS to permit tests that use compiler options that are not present for the bytecode compiler.

Xavier, if this looks ok, I will commit.

Bug report by Ben Millwood.
Patch by Jérémie Dimino and Mark Shinwell.
Steps To Reproducediff --git a/asmcomp/cmmgen.ml b/asmcomp/cmmgen.ml
index 1f640b9..7c7065d 100644
--- a/asmcomp/cmmgen.ml
+++ b/asmcomp/cmmgen.ml
@@ -1254,13 +1254,21 @@ let subst_boxed_number unbox_fn boxed_id unboxed_id box_chunk box_offset exp =
           Cassign(id, subst arg)
     | Ctuple argv -> Ctuple(List.map subst argv)
     | Cop(Cload chunk, [Cvar id]) as e ->
- if Ident.same id boxed_id && chunk = box_chunk && box_offset = 0
- then Cvar unboxed_id
- else e
+ if not (Ident.same id boxed_id) then e
+ else if chunk = box_chunk && box_offset = 0 then
+ Cvar unboxed_id
+ else begin
+ need_boxed := true;
+ e
+ end
     | Cop(Cload chunk, [Cop(Cadda, [Cvar id; Cconst_int ofs])]) as e ->
- if Ident.same id boxed_id && chunk = box_chunk && ofs = box_offset
- then Cvar unboxed_id
- else e
+ if not (Ident.same id boxed_id) then e
+ else if chunk = box_chunk && ofs = box_offset then
+ Cvar unboxed_id
+ else begin
+ need_boxed := true;
+ e
+ end
     | Cop(op, argv) -> Cop(op, List.map subst argv)
     | Csequence(e1, e2) -> Csequence(subst e1, subst e2)
     | Cifthenelse(e1, e2, e3) -> Cifthenelse(subst e1, subst e2, subst e3)
@@ -1270,7 +1278,10 @@ let subst_boxed_number unbox_fn boxed_id unboxed_id box_chunk box_offset exp =
     | Ccatch(nfail, ids, e1, e2) -> Ccatch(nfail, ids, subst e1, subst e2)
     | Cexit (nfail, el) -> Cexit (nfail, List.map subst el)
     | Ctrywith(e1, id, e2) -> Ctrywith(subst e1, id, subst e2)
- | e -> e in
+ | Cconst_int _ | Cconst_natint _ | Cconst_float _ | Cconst_symbol _
+ | Cconst_pointer _ | Cconst_natpointer _
+ | Cconst_blockheader _ as e -> e
+ in
   let res = subst exp in
   (res, !need_boxed, !assigned)
 
diff --git a/testsuite/makefiles/Makefile.one b/testsuite/makefiles/Makefile.one
index 9f95b36..6a5f5c8 100644
--- a/testsuite/makefiles/Makefile.one
+++ b/testsuite/makefiles/Makefile.one
@@ -48,7 +48,8 @@ compile: $(ML_FILES) $(CMO_FILES) $(MAIN_MODULE).cmo
     @if $(BYTECODE_ONLY); then : ; else \
       rm -f program.native program.native.exe; \
       $(MAKE) $(CMX_FILES) $(MAIN_MODULE).cmx; \
- $(OCAMLOPT) $(ADD_COMPFLAGS) -o program.native$(EXE) $(O_FILES) \
+ $(OCAMLOPT) $(ADD_COMPFLAGS) $(ADD_OPTCOMPFLAGS) \
+ -o program.native$(EXE) $(O_FILES) \
                   $(CMXA_FILES) $(CMX_FILES) $(ADD_CMX_FILES) \
                   $(MAIN_MODULE).cmx; \
     fi
diff --git a/testsuite/tests/float-unboxing/float_subst_boxed_number.ml b/testsuite/tests/float-unboxing/float_subst_boxed_number.ml
new file mode 100644
index 0000000..44271b6
--- /dev/null
+++ b/testsuite/tests/float-unboxing/float_subst_boxed_number.ml
@@ -0,0 +1,9 @@
+type t =
+ | A of float
+ | B of (int * int)
+
+let rec foo = function
+ | A x -> x
+ | B (x, y) -> float x +. float y
+
+let (_ : float) = foo (A 4.)
diff --git a/testsuite/tests/float-unboxing/float_subst_boxed_number.reference b/testsuite/tests/float-unboxing/float_subst_boxed_number.reference
new file mode 100644
index 0000000..e69de29
TagsNo tags attached.
Attached Fileshtml file icon 6670.html [^] (5,444 bytes) 2015-02-05 11:52

- Relationships
has duplicate 0006770closedshinwell inlining-related compiler crash 

-  Notes
(0013219)
shinwell (developer)
2015-02-04 09:13
edited on: 2015-02-04 09:46

The patch above was badly-formatted and missing a piece.

I've fixed the patch and pushed it to the pr6686 branch of https://github.com/mshinwell/ocaml. [^] (This is based on the upstream 4.02 branch.) I made a mistake in the commit message; the related PR is 6770.

I'm going to merge this upstream within a couple of days unless someone objects.

(0013229)
frisch (developer)
2015-02-05 11:14

Damien reported that he tracked down the issue to commit 14444. Just to be sure to understand the issue and the fix: do you agree that the problem is not strictly related to that commit, but that it made it more apparent? Or perhaps for some reasons, without the more aggressive propagation of constants, the problem could not occur?
(0013230)
shinwell (developer)
2015-02-05 11:25

I am currently building 14443 and 14444, to see what the difference in clambda code is for these examples.
(0013232)
shinwell (developer)
2015-02-05 11:52

So... the example from this PR (6686) actually works at 14444, but Damien's example (PR 6770) does not.

I attach to this issue a diff showing the difference in clambda across the 14443 -> 14444 transition for PR 6770. It looks to me as if the extra constant propagation caused the problem, but was not directly at fault; it just propagated a constant into a piece of unreachable code. Whilst it could be argued that we shouldn't have such unreachable, ill-typed code around, I think it does occur and subsequent passes (e.g. [subst_boxed_number]) need to be able to cope.

I am confident that the original example in this PR (6686) is going to be caused by approximately the same thing.
(0013239)
frisch (developer)
2015-02-05 16:18

The attached example of clambda code called for a more clever optimization. Commit 15812 (to trunk) adds better simplification for switches after inlining (it applies only when the switch subject is known constant). Its a typical case where the inlined function body can be much smaller than the original one (we already had a similar optimization for if-then-else), which also illustrates that the current criterion for inlining is not so good. The patch also shows that we could do much better if we kept track of approximations for bound values during substitutions (or more generally in the clambda representation itself).

This indirectly solves this issue, at least in the specific reported examples. But clearly your real fix is also very much welcome.
(0013445)
frisch (developer)
2015-03-11 16:37

> I'm going to merge this upstream within a couple of days unless someone objects.

I think this should go in 4.02.2. Can you commit your patch?
(0013715)
shinwell (developer)
2015-04-24 10:37

Committed to 4.02

- Issue History
Date Modified Username Field Change
2014-12-01 11:11 shinwell New Issue
2014-12-01 11:11 shinwell Status new => assigned
2014-12-01 11:11 shinwell Assigned To => shinwell
2015-02-04 09:13 shinwell Note Added: 0013219
2015-02-04 09:46 shinwell Note Edited: 0013219 View Revisions
2015-02-04 09:46 shinwell Relationship added has duplicate 0006770
2015-02-05 11:14 frisch Note Added: 0013229
2015-02-05 11:25 shinwell Note Added: 0013230
2015-02-05 11:52 shinwell Note Added: 0013232
2015-02-05 11:52 shinwell File Added: 6670.html
2015-02-05 16:18 frisch Note Added: 0013239
2015-03-11 16:37 frisch Note Added: 0013445
2015-04-24 10:37 shinwell Note Added: 0013715
2015-04-24 10:38 shinwell Status assigned => resolved
2015-04-24 10:38 shinwell Fixed in Version => 4.02.2+dev / +rc1
2015-04-24 10:38 shinwell Resolution open => fixed
2016-12-07 11:48 xleroy Status resolved => closed
2017-02-23 16:35 doligez Category OCaml backend (code generation) => Back end (clambda to assembly)
2017-02-23 16:44 doligez Category Back end (clambda to assembly) => back end (clambda to assembly)


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker