[PATCH] D157385: [clang][CFG] Cleanup functions

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

This is a great improvement. When I saw that clang now supports it and e.g. the CSA operates on the CFG, I also considered adding this.
Now I don't need to do it, so many thanks!

The code looks correct to me, except that the cleanup should be before the dtor if it has any. Other than that, the code coverage also looks great, thus I would not oppose.



================
Comment at: clang/lib/Analysis/CFG.cpp:1901-1902
       appendLifetimeEnds(Block, VD, S);
-    if (BuildOpts.AddImplicitDtors)
+    if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
       appendAutomaticObjDtor(Block, VD, S);
+    if (HasCleanupAttr)
----------------
This condition looks new. Is it an orthogonal improvement?


================
Comment at: clang/lib/Analysis/CFG.cpp:1901-1904
+    if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
       appendAutomaticObjDtor(Block, VD, S);
+    if (HasCleanupAttr)
+      appendCleanupFunction(Block, VD);
----------------
steakhal wrote:
> This condition looks new. Is it an orthogonal improvement?
Shouldn't the cleanup function run first, and then the dtor of the variable?
This way the object is already "dead" even before reaching the cleanup handler.
https://godbolt.org/z/sT65boooW


================
Comment at: clang/lib/Analysis/CFG.cpp:2113
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.
----------------
Can't this accept `const VarDecl*` instead? That way we could get rid of that `const_cast` above.


================
Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474
+// CHECK-NEXT:    3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:    4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:    5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:    6: CFGScopeEnd(f)
----------------
tbaeder wrote:
> aaronpuchert wrote:
> > Interesting test! But it seems CodeGen has them swapped: compiling this snippet with `clang -c -S -emit-llvm` I get
> > ```lang=LLVM
> > define dso_local void @_Z4testv() #0 personality ptr @__gxx_personality_v0 {
> >   %1 = alloca %class.F, align 1
> >   %2 = alloca ptr, align 8
> >   %3 = alloca i32, align 4
> >   invoke void @_Z9cleanup_FP1F(ptr noundef %1)
> >           to label %4 unwind label %5
> > 
> > 4:                                                ; preds = %0
> >   call void @_ZN1FD2Ev(ptr noundef nonnull align 1 dereferenceable(1) %1) #3
> >   ret void
> > 
> > ; ...
> > }
> > ```
> > So first cleanup, then destructor. This is with 17.0.0-rc2.
> Interesting, I thought I checked this and used the correct order. Will re-check, thanks.
I believe this demonstrates the wrong order I also spotted earlier.


================
Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};
----------------
aaronpuchert wrote:
> tbaeder wrote:
> > aaronpuchert wrote:
> > > As with the cleanup function, a definition shouldn't be necessary.
> > Is there a way to test whether the contents of the cleanup function are being checked as well? From these tests, I only know we consider them called, but not whether we (properly) analyze their bodies in the context as well. Or is that separate from this patch?
> For now we're just adding a new element to the CFG and adapting the respective tests. The CFG is generated on a per-function basis, and the generation of one function's CFG will never look into another function's body. It might use some (declaration) properties of course, like whether it has `[[noreturn]]` or `noexcept`. Of course we should also generate a CFG for the cleanup function, but that's independent of this change.
> 
> Users of the CFG will naturally need to be taught about this new element type to “understand” it. Otherwise they should simply skip it. Since the CFG previously did not contain such elements, there should be no change for them. So we can also initially just add the element and not tell anybody about it.
> 
> We could also add understanding of the new element type to other CFG users, but you don't have to do this. If you only care about Thread Safety Analysis, then it's totally fine to handle it only there.
> 
> But let's move all changes to Thread Safety Analysis into a follow-up, so that we don't have to bother the CFG maintainers with that.
Speaking of `noreturn`, I think it is worth demonstrating that if the cleanup function has `noreturn` attribute, then the dtor is not called.


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

https://reviews.llvm.org/D157385