Page MenuHomePhabricator

[CFG] Make destructor calls more accurate

Authored by comex on Aug 18 2019, 4:16 PM.



Note: I don't have commit access.

Various changes to reduce discrepancies in destructor calls between the
generated CFG and the actual codegen, along with a new test file.

In particular:

  • Respect C++17 copy elision; previously it would generate destructor calls for elided temporaries, including in initialization and return statements.
  • Don't generate duplicate destructor calls for statement expressions.
  • Fix initialization lists.
  • Fix comma operator.
  • Change printing of implicit destructors to print the type instead of the class name directly, matching the code for temporary object destructors. The class name was blank for lambdas.

Also update some existing tests which were broken by the changed formatting
and/or the fixed behavior.

Implementation notes:

  • Rename BindToTemporary to ExternallyDestructed, which more accurately reflects what the parameter does and matches the naming in CodeGen.
  • Change VisitChildrenForTemporaryDtors to take an ExternallyDestructed parameter, for the sake of InitListExprs, which have an arbitrary number of children and may or may not be externally destructed.
  • Add a function VisitExternallyDestructed with the right behavior for return statements and statement expressions.

The new test file also includes tests for some preexisting buggy cases which
this patch does *not* fix. What a mess...

Diff Detail


Event Timeline

comex created this revision.Aug 18 2019, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2019, 4:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Szelethus edited reviewers, added: NoQ; removed: dergachev.a.Aug 18 2019, 4:32 PM
NoQ added a comment.Aug 18 2019, 10:18 PM

Whoaaa, at a glance this looks absolutely fantastic. I'll get back to this tomorrow to take a more careful look.

NoQ added a comment.Aug 19 2019, 4:55 PM

I have a few nitpicks but i still love this patch, thank you! It picks up the work exactly where i dropped it a year or so ago.

Respect C++17 copy elision; previously it would generate destructor calls for elided temporaries, including in initialization and return statements.

I think the root cause of these redundant destructors was the confusing AST that contains a CXXBindTemporaryExpr even when there's not much of a temporary. See also a very loosely related discussion in

2102 ↗(On Diff #215807)

This goes over the 80 characters limit :)

2290–2295 ↗(On Diff #215807)

This looks like a copy-paste from VisitExprWithCleanups with the ExternallyDestructed flag flipped. Could you double-check if the update to asc that's present in VisitExprWithCleanups is relevant here as well? I've actually no idea what do these do and we don't seem to have any tests for them, so feel free to ignore, but it might make sense to at least deduplicate the code.

415 ↗(On Diff #215807)

Yay!! I'm very happy these are sorted out.

63 ↗(On Diff #215807)

Whoops, looks like i forgot to add a FIXME here. This update is correct, thanks!

3 ↗(On Diff #215807)

Did you mean t.20? :)

50 ↗(On Diff #215807)

Maybe also add CXX20-NOT so that to make sure that the destructor *isn't* there? (i think this applies to a lot of other tests)

93 ↗(On Diff #215807)

Pls mention somehow that this language feature is called "generalized lambda captures" because it's fairly hard to google for :)

233 ↗(On Diff #215807)

I'm afraid that nobody will notice this comment when more items will be added to this test file. Having a FIXME in tests themselves is sufficient.

251–252 ↗(On Diff #215807)

Is it just mismatching dump styles or is it an actual error?

252–254 ↗(On Diff #215807)

These should most likely be two separate loops: default arguments are destroyed immediately after the constructor of qux[2], but elements of qux should not be destroyed before the end of the scope.

273 ↗(On Diff #215807)

Feel free to move this into test/Analysis/inputs/system-header-simulator-cxx.h, we could always use more mocks in there.

836 ↗(On Diff #215807)

Yay!! I'm very happy these are sorted out.

comex abandoned this revision.Wed, Aug 21, 8:06 PM
comex marked 18 inline comments as done.
comex added inline comments.
2102 ↗(On Diff #215807)

Thanks, fixed.

2290–2295 ↗(On Diff #215807)

Oops. I omitted it because the function is always called with AlwaysAdd, but should have removed the second parameter to reflect that. However, on reflection, I think it would be better to simply add an ExternallyDestructed parameter to Visit and VisitExprWithCleanups. I'll do that in the updated patch.

3 ↗(On Diff #215807)

Yep, will fix.

50 ↗(On Diff #215807)

The -implicit-check-not=destructor handles that: it complains if any line containing "destructor" isn't covered by a CHECK.

93 ↗(On Diff #215807)

Okay :)

233 ↗(On Diff #215807)

Okay, I'll remove it.

251–252 ↗(On Diff #215807)

Oops – it's actually just the printing code for implicit destructors that does:

if (const ArrayType *AT = ACtx.getAsArrayType(T))
  T = ACtx.getBaseElementType(AT);

I'm not sure why that's there: perhaps because ~Foo[]() is not valid C++ syntax, but there's plenty of other places in the printing code that can print invalid syntax, and printing just ~Foo() is misleading.
For now, until this code is overhauled to emit loops for constructors and destructors, I think it makes the most sense to remove those two lines. I'll do that in the updated patch.

252–254 ↗(On Diff #215807)

Yeah, that's what I meant. I'll clarify the text.

273 ↗(On Diff #215807)

Well, TestPromise is a bit specific to this test. In particular, the return_value method would normally return void, but I have it return a Foo, which just gets discarded, in order to check that the corresponding destructor call is recorded properly.

The non-test-specific boilerplate is <10 lines of code. I could add it to system-header-simulator-cxx.h, but then I'd have to figure out how to make sure that this test file doesn't get confused by CFG output from any inline functions defined there, especially given the -implicit-check-not flag. Right now almost all function implementations in the header are within a template and thus won't produce CFG (or IR or other analysis) output by themselves, so the solution would probably consist of merely adding a comment to system-header-simulator-cxx.h, requesting that any future additions be either similarly templated or hidden behind an #ifdef. That's definitely feasible, but it doesn't seem worth dealing with for such a small amount of code. :)

However, I need to fix the indentation and improve the wording of the comment in coreturn.

comex updated this revision to Diff 216545.Wed, Aug 21, 8:12 PM
comex marked 9 inline comments as done.

Changes since last version:

  • Rebased.
  • Added ExternallyDestructed parameter to VisitExprWithCleanups and CFGBuilder::Visit; removed VisitExternallyDestructed.
  • Changed CFG printing to show when the type being destructed is an array.
  • Fixed temporary filename mismatch in test.
  • Improved comments to address feedback.
NoQ accepted this revision.Thu, Aug 22, 3:41 PM

This looks fantastic, thanks!

Note: I don't have commit access.

I can commit this for you but i believe you should totally ask for it.

50 ↗(On Diff #215807)

Ahaa, i see what you did here!

251–252 ↗(On Diff #215807)

I don't care if it's valid C++ syntax as long as it's readable/testable.

Another possible variant is ~Foo() (Array destructor), we seem to do this a lot.

273 ↗(On Diff #215807)

Ok np then!

This revision is now accepted and ready to land.Thu, Aug 22, 3:41 PM

Heh, I guess I'll request commit access, although I'm not sure if I have enough of a 'track record of submitting high quality patches'. But for now, can you commit this? Thanks :)

Heh, I guess I'll request commit access, although I'm not sure if I have enough of a 'track record of submitting high quality patches'.

You really do :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 28, 11:45 AM

This commit causes some warnings in the Linux kernel that appear to be false positives. For example:

  CC      drivers/base/cpu.o
drivers/base/cpu.c:404:1: error: control may reach end of non-void function [-Werror,-Wreturn-type]
1 error generated.

Corresponds to this function:

struct device *get_cpu_device(unsigned cpu)
	if (cpu < nr_cpu_ids && cpu_possible(cpu))
		return per_cpu(cpu_sys_devices, cpu);
		return NULL;

creduce spits out:

a() {
  if (b())
    return ({
      while (0)
    return 0;
NoQ added a comment.Thu, Aug 29, 1:36 PM

Thanks @nathanchance! I think i fixed it in rC370406.


This is basically what this comment was about. We're building the CFG from bottom to top, so when the return-value expression has a non-trivial CFG on its own, we need to continue building from the entry to the return-value expression CFG rather than from the block to which we've just appended the return statement.


Yeah, something broke.

Fig. 1. Before.

Fig. 2. After.

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index e4ed0f86b91..5f2f46413b9 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -2996,7 +2996,7 @@ CFGBlock *CFGBuilder::VisitReturnStmt(Stmt *S) {
   if (ReturnStmt *RS = dyn_cast<ReturnStmt>(S)) {
     Expr *O = RS->getRetValue();
     if (O)
-      Visit(O, AddStmtChoice::AlwaysAdd, /*ExternallyDestructed=*/true);
+      return Visit(O, AddStmtChoice::AlwaysAdd, /*ExternallyDestructed=*/true);
     return Block;
   } else { // co_return
     return VisitChildren(S);

Fig. 3. Suggested fix.

@NoQ thank you for the fix, I can confirm it works!