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

Don't delete let bodies in Cmmgen #959

Merged
merged 5 commits into from Dec 9, 2016

Conversation

Projects
None yet
4 participants
@mshinwell
Contributor

mshinwell commented Dec 9, 2016

(was PR#7427)

Cmmgen is sometimes relied upon to do part of the work of lifting constant closures. When using Closure this happens all the time: Closure rewrites references to constant closures to symbols, but Cmmgen does the actual lifting (i.e. associating symbols with their definitions). This aspect of Cmmgen is relied on less when using Flambda but is still used sometimes.

For this example:

module F() = struct
  module M = struct
    let aaa = assert false
    let bbb () = assert false
  end
  let ccc () = M.bbb ()
end

compilation fails because Cmmgen.transl_let deletes let bbb () = assert false and thus the symbol for this constant closure never has a definition. This happens because the defining expression of let aaa never terminates and the code currently ignores the body in such cases. This is wrong because the body must still be traversed to do this lifting.

The easiest fix seems to be just to delete this particular case. In the case where there really is dead code after a non-terminating instruction I think the subsequent backend passes are going to clear it up anyway.

@mshinwell mshinwell added the bug label Dec 9, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 9, 2016

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

Looks good to me (both the diagnostic and the fix). Could you (or someone else) squash the 4 commits into one? Then it can be merged.

Contributor

xavierleroy commented Dec 9, 2016

Looks good to me (both the diagnostic and the fix). Could you (or someone else) squash the 4 commits into one? Then it can be merged.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 9, 2016

Contributor

Does anyone understand the (probably unrelated) AppVeyor test failures?

Contributor

alainfrisch commented Dec 9, 2016

Does anyone understand the (probably unrelated) AppVeyor test failures?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 9, 2016

Contributor

Fairly sure the AppVeyor failure is because of testsuite fixes which are only on 4.04 branch... I got partway through a rebase of 4.04 on to trunk last night, but I won't be able to finish it until tomorrow (if anyone wishes to do it today!)

Contributor

dra27 commented Dec 9, 2016

Fairly sure the AppVeyor failure is because of testsuite fixes which are only on 4.04 branch... I got partway through a rebase of 4.04 on to trunk last night, but I won't be able to finish it until tomorrow (if anyone wishes to do it today!)

@mshinwell mshinwell merged commit 4c13a3a into ocaml:trunk Dec 9, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

nojb added a commit to nojb/ocaml that referenced this pull request Dec 29, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment