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

Flambda: prevent recursive approximations in Build_export_info #8924

Merged
merged 5 commits into from Sep 11, 2019

Conversation

@lthls
Copy link
Contributor

commented Sep 6, 2019

This is a quick-and-dirty fix for the bug reported in #8921.
I think it's enough to cover all problematic cases (I don't think Let_rec in expressions is problematic).
I'll try to reuse the env parameter instead of introducing a new one, but functionally the current code is what I have in mind.

@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I've pushed a less hackish implementation that reuses the environment. It feels weird because there are two kinds of environments (global and local), and we only have a global one at the Let_rec_symbol point and only a local one at the approx_of_constant_defining_value_block_field point, but I've managed to fit it somehow.

@chambart

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

I would have preferred a solution that didn't worsen the approximation, but I was convinced by the fact that approximations coming from other files are more precise than those from the current file.

I contemplated for a moment 'the rigth fix' that made all the cases handle recursive symbols properly, but to be honest, this probably doesn't matter at all. So this fix is the right one, I have some nit-pick that I'll do as a PR on this.

chambart and others added 2 commits Sep 9, 2019
@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I've merged @chambart's PR. @lpw25, any thoughts on this are welcome.
I've preserved the commit history so far, but this PR is meant to be squashed and merged.

@lpw25
lpw25 approved these changes Sep 10, 2019
Copy link
Contributor

left a comment

LGTM

@chambart chambart merged commit f814450 into ocaml:trunk Sep 11, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Do we want to backport this fix in 4.09?
(It looks safe enough to me.)

@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

It's safe to backport. I think the existence of a workaround means that backporting for old releases is not a priority, but there's no reason it can't go in 4.09.

gasche added a commit that referenced this pull request Sep 11, 2019
Fixes #8921

(cherry picked from commit f814450)
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Backported in 0671715.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.