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

subst_boxed_number sometimes fails to record that a boxed value is needed #6686

Closed
vicuna opened this issue Dec 1, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Dec 1, 2014

Original bug ID: 6686
Reporter: @mshinwell
Assigned to: @mshinwell
Status: closed (set by @xavierleroy on 2016-12-07T10:48:55Z)
Resolution: fixed
Priority: high
Severity: crash
Version: 4.02.1
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: back end (clambda to assembly)
Has duplicate: #6770
Monitored by: @diml jmeber

Bug description

The 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 reproduce

diff --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

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 4, 2015

Comment author: @mshinwell

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 5, 2015

Comment author: @alainfrisch

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?

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 5, 2015

Comment author: @mshinwell

I am currently building 14443 and 14444, to see what the difference in clambda code is for these examples.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 5, 2015

Comment author: @mshinwell

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 5, 2015

Comment author: @alainfrisch

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 11, 2015

Comment author: @alainfrisch

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?

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 24, 2015

Comment author: @mshinwell

Committed to 4.02

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.