Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007680OCamlmiddle end (typedtree to clambda)public2017-11-28 16:572017-12-06 07:57
Reportervlaviron 
Assigned Tofrisch 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.06.0 
Target VersionFixed in Version4.07.0+dev 
Summary0007680: Incorrect interaction between Matching.for_let and Simplif.simplify_exits
DescriptionCompilation of match ... with exception ... uses Lambda.next_negative_raise_count instead of Lambda.next_raise_count because it generates a Lstaticraise that jumps from inside a Ltrywith block to outside it, and Simplif.simplify_exits is not allowed to move the handler's code inside the trywith block.
The optimisation for compilation of let-bindings of tuples in Matching.for_let can also generate Lstaticraise that jumps outside a Ltrywith block, but it doesn't use a negative raise count so in some cases Simplif.simplify_exits can produce incorrect code.
Steps To Reproduce$ cat bug.ml
let f () =
  let (a,b) =
    try (1,2)
    with _ -> assert false
  in
  if a + b = 3 then raise Not_found

let _ = try f () with Not_found -> print_endline "Ok"
$ ocamlc -o bug bug.ml
$ ./bug
Fatal error: exception Assert_failure("bug.ml", 4, 14)
Additional InformationFound while updating GPR#1482.
The bug is present since 4.03.0, and the corresponding feature is listed in Changes as 'PR#4800: better compilation of tuple assignment'.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0018703)
gasche (developer)
2017-11-28 18:24
edited on: 2017-11-28 18:24

Are you planning to send a github pull request (GPR) to fix this, or should we take care of it? (Is it correct to just use negative numbers in the code produced by this optimization?)

(0018706)
vlaviron (reporter)
2017-11-28 19:04

I have this fixed as part of my work on exceptions in the backend, but I don't plan to make a specific pull request for this bug.
Using negative numbers in the code fixes the issue, but possibly leads to some regressions in terms of missed optimizations (for example, `let (a,b) = (1,2) in a + b` would not be simplified into `1 + 2`).
(0018710)
frisch (developer)
2017-11-29 09:48

I guess dropping the case for Ltrywith in Matching.map_return would fix the problem.

Is it worth using a more complex logic to maintain the optimization in presence of try-with as in the reported example?
(0018715)
vlaviron (reporter)
2017-11-29 11:53

I'm not sure it is sound to drop only one case: if an other branch activates the optimization, you would end up with the trywith branch returning a couple and not triggering the handler, which is just plain wrong.
Disabling the optimization by raising an exception in the Ltrywith case would be better, but for (almost) full compatibility you could use two nfail numbers (a positive one and a negative one) and pass two functions f_pos and f_neg to map_return (using the two nfails). You would then call f_pos on the default case, and use ''map_return f_pos f_neg l'' in the recursive calls except the first one in Ltrywith where you would use ''map_return f_neg f_neg l1''.
Nesting the negative catch inside the positive one should allow Simplif.simplify_exits to recover the original code when there are no trywiths.

Or alternatively, you could look at what I'm doing on commit da8a7b4 of GPR#1482: it replaces negative numbers in static exceptions with annotations on Lstaticraise, and as a result the fix gets much cleaner.
(0018716)
frisch (developer)
2017-11-29 12:00

> you would end up with the trywith branch returning a couple and not triggering the handler, which is just plain wrong.

I'm not sure to follow. Removing the case for Ltrywith would have the effect of considering the whole try...with sub-expression to be "opaque" w.r.t. the transformation (as e.g. a function call). The sub expression would not be rewritten, and would simply return a tuple or raise an exception (in the same way a function call could raise an exception).
(0018717)
vlaviron (reporter)
2017-11-29 12:06

Ah, yes, you're right (I don't know why I supposed that it would mean that Ltrywith would get the Lstaticraise treatment).
(0018718)
frisch (developer)
2017-11-29 12:40

FWIW, the test in testsuite/asmcomp/bind_tuples.ml explicitly relies on the optimization going under try...with.
(0018719)
frisch (developer)
2017-11-29 13:51

Working on a fix in simplif.mf, where we keep track of the try..with nesting to avoid inlining a static handler across a try..with.
(0018721)
frisch (developer)
2017-11-29 15:56

https://github.com/ocaml/ocaml/pull/1497 [^]

- Issue History
Date Modified Username Field Change
2017-11-28 16:57 vlaviron New Issue
2017-11-28 18:24 gasche Note Added: 0018703
2017-11-28 18:24 gasche Note Edited: 0018703 View Revisions
2017-11-28 19:04 vlaviron Note Added: 0018706
2017-11-29 09:48 frisch Note Added: 0018710
2017-11-29 11:53 vlaviron Note Added: 0018715
2017-11-29 12:00 frisch Note Added: 0018716
2017-11-29 12:06 vlaviron Note Added: 0018717
2017-11-29 12:40 frisch Note Added: 0018718
2017-11-29 13:51 frisch Note Added: 0018719
2017-11-29 15:56 frisch Note Added: 0018721
2017-12-06 07:57 frisch Status new => resolved
2017-12-06 07:57 frisch Fixed in Version => 4.07.0+dev
2017-12-06 07:57 frisch Resolution open => fixed
2017-12-06 07:57 frisch Assigned To => frisch


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker