tbaeder marked 2 inline comments as done.
tbaeder added inline comments.
================
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.
----------------
steakhal wrote:
> Can't this accept `const VarDecl*` instead? That way we could get rid of that
> `const_cast` above.
Yes, I just didn't want to modify the existing function signature.
================
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)
----------------
steakhal wrote:
> 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.
The current code generates the given CFG, i.e. the cleanup function comes
first, followed by the dtor:
```
[B1]
1: CFGScopeBegin(f)
2: (CXXConstructExpr, [B1.3], F)
3: F f __attribute__((cleanup(f_)));
4: CleanupFunction (f_)
5: [B1.3].~F() (Implicit destructor)
6: CFGScopeEnd(f)
Preds (1): B2
Succs (1): B0
```
(The comment from Aaron is from before when I had the order swapped.)
================
Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+ ~F() {}
+};
----------------
steakhal wrote:
> 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.
Yeah, that does not work correctly right now.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157385/new/
https://reviews.llvm.org/D157385
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
