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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

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 <https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access>.



================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:50
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
----------------
comex wrote:
> 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.
Ahaa, i see what you did here!


================
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
----------------
comex wrote:
> 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.
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.


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:273
+#if CXX2A
+// Boilerplate needed to test co_return:
+
----------------
comex wrote:
> 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`.
Ok np then!


Repository:
  rC Clang

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

https://reviews.llvm.org/D66404