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

Incorrect interaction between Matching.for_let and Simplif.simplify_exits #7680

Closed
vicuna opened this Issue Nov 28, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Nov 28, 2017

Original bug ID: 7680
Reporter: vlaviron
Assigned to: @alainfrisch
Status: resolved (set by @alainfrisch on 2017-12-06T06:57:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.06.0
Fixed in version: 4.07.0+dev/beta2/rc1/rc2
Category: middle end (typedtree to clambda)
Monitored by: @alainfrisch

Bug 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) =
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 information

Found while updating #1482.
The bug is present since 4.03.0, and the corresponding feature is listed in Changes as '#4800: better compilation of tuple assignment'.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 28, 2017

Comment author: @gasche

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?)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 28, 2017

Comment author: vlaviron

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).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: @alainfrisch

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?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: vlaviron

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 #1482: it replaces negative numbers in static exceptions with annotations on Lstaticraise, and as a result the fix gets much cleaner.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: @alainfrisch

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).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: vlaviron

Ah, yes, you're right (I don't know why I supposed that it would mean that Ltrywith would get the Lstaticraise treatment).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: @alainfrisch

FWIW, the test in testsuite/asmcomp/bind_tuples.ml explicitly relies on the optimization going under try...with.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: @alainfrisch

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 29, 2017

Comment author: @alainfrisch

#1497

@vicuna vicuna closed this Dec 6, 2017

@vicuna vicuna added the middle-end label Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

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.