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

Code in defer block is run after deinitializer #14797

Closed
dlongnecke-cray opened this issue Jan 22, 2020 · 2 comments · Fixed by #14804
Closed

Code in defer block is run after deinitializer #14797

dlongnecke-cray opened this issue Jan 22, 2020 · 2 comments · Fixed by #14804
Assignees

Comments

@dlongnecke-cray
Copy link
Contributor

dlongnecke-cray commented Jan 22, 2020

Summary of Problem

The code contained in a defer block is run after deinitializers for any local variables created in the same scope.

I expect the opposite to be true (defer blocks run first).

Steps to Reproduce

Reproducer courtesy of @mppf:

record R {
  var x: int;
  proc init() { this.x = 1; writeln("init 1"); }
  proc deinit() { writeln("deinit ", this.x); }
}

proc test() {
  var rec = new R();
  defer { writeln("defer setting 0"); rec.x = 0; }
  writeln("created rec ", rec);
  writeln("end");
}

test();

Note that taking a reference to the variable or avoiding use of a defer block both seem to fix this issue.

@bradcray
Copy link
Member

To make sure I'm understanding: The assertion is that it's more correct to have the defer block run first and then the local variable initialization, is that right?

It seems this was the case in 1.20 (TIO). Has it changed since then?

@mppf
Copy link
Member

mppf commented Jan 23, 2020

Yes. The issue is that rec in the example is deinited "after last mention" which, today on master (after my recent deinit changes), the compiler thinks is right before writeln("end"). But, since rec is mentioned in the defer block, the compiler needs to keep it alive until the defer block runs. To fix, I'm expecting to adjust the compiler to consider a use in a defer block to make a variable expire at the end of the block. This will not be hard to adjust.

mppf added a commit to mppf/chapel that referenced this issue Jan 23, 2020
* Resolves issue chapel-lang#14797
* Adds testing
* Uses slightly faster C++ std::map calls in a few places and avoids
  adding to the map for outer variables.
mppf added a commit that referenced this issue Jan 23, 2020
Improve expiring value analysis

This PR makes two improvements to expiring value analysis.

* It updates the analysis to consider variables mentioned in a defer 
  statement to expire at the end-of-block. This resolves issue #14797.
* It changes the logic for inserting into the map detecting captures. 
  Now, a capture will not be detected before the variable it is
  describing is initialized. This resolves a problem I had not noticed
  with the I/O example from issue #11492. I also updated the test to
  output the array that was read, so that it will be more obvious if this
  pattern fails again.

Resolves #14797.

Reviewed by @benharsh - thanks!

- [x] full local testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants