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

Do not delete unused closures in un_anf.ml #998

Merged
merged 3 commits into from Feb 15, 2017

Conversation

Projects
None yet
4 participants
@lpw25
Contributor

lpw25 commented Jan 6, 2017

The following code:

let main x =
  let[@inline never] inner () =
    let[@inline never] foo y () () () () () () () = x + y in
    let x1, x2, x3 = x + 1, x + 2, x + 3 in
    let bar p y () () () =
      if p then foo y () () () () () () ()
      else x1 + x2 + x3
    in
    let[@inline never] baz0 y () () () () () () () =
      let y1 = y + 1 in
      let[@inline never] baz1 () () () () () =
        bar false y1 () () ()
      in
      baz1 () () () () ()
    in
    baz0 1 () () () () () () ()
  in
  inner ()

compiled using flambda and -unbox-closures will produce an error from the system linker:

bar.o: In function `camlBar__bar_152':
:(.text+0x144): undefined reference to `camlBar__foo_13'
collect2: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking

(The bug is actually architecture dependent: on architectures other than amd64 you'll need to adjust the number of () in the above code to get the error).

The issue is the following snippet from un_anf.ml:

| (Constant | Moveable | Moveable_not_into_loops), _, false, _ ->
       (* A moveable expression that is never used may be eliminated. *)
       un_anf_and_moveable ident_info env body

which allows unused closures (the only thing that still produces Moveable_not_into_loops) to be deleted. However, it is not in general safe to delete closure expressions from clambda code.

The fix is simply to take the Moveable_not_into_loops case out of that pattern. However, after the fix Moveable_not_into_loops is functionally identical to Fixed, so the PR goes further and removes Moveable_not_into_loops entirely.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 6, 2017

Contributor

I think you should probably add the testcase to the testsuite even though it's a bit fragile.

Contributor

mshinwell commented Jan 6, 2017

I think you should probably add the testcase to the testsuite even though it's a bit fragile.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Jan 6, 2017

Contributor

A bit fragile is an understatement. I suspect that almost any significant change in flambda will silently cause this file to stop testing the problem. Without being able to test specific flambda output or clambda input I cannot see how to write a proper test for this issue.

Contributor

lpw25 commented Jan 6, 2017

A bit fragile is an understatement. I suspect that almost any significant change in flambda will silently cause this file to stop testing the problem. Without being able to test specific flambda output or clambda input I cannot see how to write a proper test for this issue.

@mshinwell mshinwell added the bug label Jan 6, 2017

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Jan 9, 2017

Contributor

Ok. I've added the test with a comment warning about how very fragile it is.

Contributor

lpw25 commented Jan 9, 2017

Ok. I've added the test with a comment warning about how very fragile it is.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 17, 2017

Contributor

This looks OK to me. @chambart should review too.

Contributor

mshinwell commented Jan 17, 2017

This looks OK to me. @chambart should review too.

@chambart

This comment has been minimized.

Show comment
Hide comment
@chambart

chambart Jan 25, 2017

Contributor

The change is safe

Contributor

chambart commented Jan 25, 2017

The change is safe

@chambart chambart added the approved label Jan 25, 2017

@mshinwell mshinwell merged commit c1828b9 into ocaml:trunk Feb 15, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Feb 15, 2017

Contributor

Just of note:
https://developers.redhat.com/blog/2017/02/13/testing-testing-gcc/
describes how gcc handles test for there various internal representations and optimizations.

Contributor

bschommer commented Feb 15, 2017

Just of note:
https://developers.redhat.com/blog/2017/02/13/testing-testing-gcc/
describes how gcc handles test for there various internal representations and optimizations.

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