|Anonymous | Login | Signup for a new account||2017-12-12 17:12 CET|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0007680||OCaml||middle end (typedtree to clambda)||public||2017-11-28 16:57||2017-12-06 07:57|
|Target Version||Fixed in Version||4.07.0+dev|
|Summary||0007680: Incorrect interaction between Matching.for_let and Simplif.simplify_exits|
|Description||Compilation 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) =
with _ -> assert false
if a + b = 3 then raise Not_found
let _ = try f () with Not_found -> print_endline "Ok"
$ ocamlc -o bug bug.ml
Fatal error: exception Assert_failure("bug.ml", 4, 14)
|Additional Information||Found 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'.
|Tags||No tags attached.|
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?)
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`).
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?
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.
> 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).
|Ah, yes, you're right (I don't know why I supposed that it would mean that Ltrywith would get the Lstaticraise treatment).|
|FWIW, the test in testsuite/asmcomp/bind_tuples.ml explicitly relies on the optimization going under try...with.|
|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.|
|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|