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

Fixed issue #426 — dtor / destructor not called for (rvalue) struct used in opApply #427

Merged
merged 5 commits into from Jul 20, 2013

Conversation

AlexeyProkhin
Copy link
Member

Fixing the bug turned out to be a lot more work than I expected.

The first obstacle I encountered was that IRLandingPad class we use to generate exception handling code expected finally block to be dmdfe's Statement. Which temp variables destructor calls are not. So in order to fix the issue, I had to extend IRLandingPad API a little bit to have more control over eh codegen.

Once I had the updated API at my disposal, I made the first attempt at fixing the issue. I followed @ibuclaw suggestion and wrapped the destructor calls in Expression::toElemDtor in try-finally. While it fixed the issue at hand, the number of generated landing pads had been highly increased. Essentially, it was not possible to write any code without triggering eh codegen.

And here goes the third part of my patch. As you probably guessed, the majority of Expression::toElemDtor() calls does not need to call any destructors. Previously, we had a classical chicken-and-egg problem: in order to decide whether or not a try-finally is required, we needed a number of temporary variables, but to get it, we had to run the expression codegen at which point we had to know whether a try-finally is required. As a solution, I created an Expression tree walker that looks for the temporary variables before the codegen.

Just laying down the ground work before fixing issue 426.
… it is required

(i.e. there are actually some destructor calls that are needed to be put into finally)
@ibuclaw
Copy link
Contributor

ibuclaw commented Jul 10, 2013

Once I had the updated API at my disposal, I made the first attempt at fixing the issue. I followed @ibuclaw suggestion and wrapped the destructor calls in Expression::toElemDtor in try-finally. While it fixed the issue at hand, the number of generated landing pads had been highly increased. Essentially, it was not possible to write any code without triggering eh codegen.

Yep, I had limited it to TOKcall's myself. As you also get cases (mostly TOKcomma's) where asserts could be thrown during the initialisation of the temporary, so the temporary is left in an invalid state, and because of finally { } the destructor is called on the temporary despite never being initialised!

@AlexeyProkhin
Copy link
Member Author

Yep, I had limited it to TOKcall's myself. As you also get cases (mostly TOKcomma's) where asserts could be thrown during the initialisation of the temporary, so the temporary is left in an invalid state, and because of finally { } the destructor is called on the temporary despite never being initialised!

@ibuclaw, thanks again. Per your suggestion, I had limited it to TOKCall's as well. I didn't have to special-case TOKcomma's, though.

@ibuclaw
Copy link
Contributor

ibuclaw commented Jul 10, 2013

oh, TOKcomma special case is just makes the codegen neater, and is not needed.
pretty much replaces:
cast(type) save_expr<very long expression, var>, dtors(), save_expr<very long expression, var>;
with:
cast(type) very long expression, dtors(), var;
Didn't check if it affects assembly, but it produces less temporaries during the gimplification. =)

redstar added a commit that referenced this pull request Jul 20, 2013
Fixed issue #426 — dtor / destructor not called for (rvalue) struct used in opApply
@redstar redstar merged commit 795df4d into ldc-developers:master Jul 20, 2013
@AlexeyProkhin AlexeyProkhin deleted the issue426 branch July 20, 2013 19:14
redstar pushed a commit that referenced this pull request Sep 27, 2014
replace rt.qsort with libc wrapper
@dnadlinger
Copy link
Member

This probably caused #795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants