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

Fix match miscompilation with flambda #2239

Merged
merged 3 commits into from Feb 11, 2019

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

commented Feb 8, 2019

The translation of the switch construct in cmmgen.ml assumes that the argument to a switch can safely be discarded if it is unused. This (undocumented) invariant does not hold for code coming from flambda resulting in side-effecting expressions being incorrectly discarded. This PR fixes the issue by binding the argument of the switch construct to ensure that side-effecting expressions are not discarded.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

LGTM. If you have a repro case, can you add it as a non-regression test?

@alainfrisch alainfrisch added the bug label Feb 8, 2019

@lpw25 lpw25 force-pushed the lpw25:fix-match-miscompilation-with-flambda branch from 3df2cf6 to 04a0cbb Feb 8, 2019

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

I've added a test and a Changes entry.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

For the record, here is @lpw25 's explanation as to when this bug can be hit:

Essentially you need a match where every case is on a non-immediate constructor and where the bodies of the cases are not identical but will become identical once flambda has optimised them. The value being matched must be produced by a side-effecting expression where the result is not constant and the side-effects cannot be lifted out of the expression. In such a case the expression will be incorrectly discarded losing the side-effect.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@alainfrisch This should be ready for approving now.

@chambart

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

LGTM too. I merge. This must have been quite tricky to track !

@chambart chambart merged commit 0779456 into ocaml:trunk Feb 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chambart

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

By the way that's a simple bugfix. I vote for cherry-picking into 4.08. @damiendoligez any objection ?

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Cherry-picked to 4.08 as 10b5175..e54ea86

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.