[PATCH] D66404: [CFG] Make destructor calls more accurate

Wed Aug 21 20:09:19 PDT 2019

comex abandoned this revision.
comex marked 18 inline comments as done.
comex added inline comments.

================
Comment at: lib/Analysis/CFG.cpp:2102
     case Stmt::CompoundStmtClass:
-      return VisitCompoundStmt(cast<CompoundStmt>(S));
+      return VisitCompoundStmt(cast<CompoundStmt>(S), /*ExternallyDestructed=*/false);

----------------
NoQ wrote:
> This goes over the 80 characters limit :)
Thanks, fixed.

================
Comment at: lib/Analysis/CFG.cpp:2290-2295
+    if (BuildOpts.AddTemporaryDtors) {
+      TempDtorContext Context;
+      VisitForTemporaryDtors(EC->getSubExpr(),
+                             /*ExternallyDestructed=*/true, Context);
+    }
+    return Visit(EC->getSubExpr(), asc);
----------------
NoQ wrote:
> 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.
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.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:3
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14 -DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a -DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.17 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14 -implicit-check-not=destructor %s
----------------
NoQ wrote:
> Did you mean `t.20`? :)
Yep, will fix.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:50
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
----------------
NoQ wrote:
> 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)
The `-implicit-check-not=destructor` handles that: it complains if any line containing "destructor" isn't covered by a CHECK.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:93
+
+void elided_lambda_capture_init() {
+  // The copy from get_foo() into the lambda should be elided.  Should call
----------------
NoQ wrote:
> Pls mention somehow that this language feature is called "generalized lambda captures" because it's fairly hard to google for :)
Okay :)

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:233
+
+// FIXME: Here are some cases that are currently handled wrongly:
+
----------------
NoQ wrote:
> 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.
Okay, I'll remove it.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:251-252
+void default_ctor_with_default_arg() {
+  // FIXME: The CFG records a construction of the array type but a destruction
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of
----------------
NoQ wrote:
> Is it just mismatching dump styles or is it an actual error?
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.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:252-254
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of
+  // default arguments.
----------------
NoQ wrote:
> 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.
Yeah, that's what I meant.  I'll clarify the text.

================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:273
+#if CXX2A
+// Boilerplate needed to test co_return:
+
----------------
NoQ wrote:
> Feel free to move this into `test/Analysis/inputs/system-header-simulator-cxx.h`, we could always use more mocks in there.
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`.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66404/new/

https://reviews.llvm.org/D66404