[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Analysis/CFG.cpp:1945
+      auto DRE =
+          DeclRefExpr::Create(*Context, {}, {}, VD, false, SourceLocation(),
+                              VD->getType(), VK_PRValue);
----------------
tbaeder wrote:
> NoQ wrote:
> > Is there a way to attach some valid source locations here, like the source location of the attribute's identifier argument?
> > 
> > I'm worried that the static analyzer may be unable to consume the CFG without them, it typically requires source locations for almost arbitrary statements. I'm still not sure how it'll react to synthetic statements, maybe turning them into a custom `CFGElement` subclass (like destructors) would be safer as it'll let all clients of the CFG to treat these in a custom manner. Otherwise it makes it look like a completely normal call-expression to the clients, which is not necessarily a good thing.
> Look like I can't query the attribute for any locations except its entire range; I can also just pass `VD->getLocation()` to all locations here.
> 
> I thought doing this without creating fake AST nodes is better as well but I wasn't sure how to do that properly, or if I have to update all consumers of the CFG.
Based on the other comment thread, yeah I think it's better to have a new CFG element class than to build synthetic AST. The new element would replace like 5-6 elements that the synthetic AST would otherwise imply. And it'll have the benefit of explicitly indicating that this isn't a "normal" call-expression, which is good because it's likely that many clients actually want *at least a little bit* of custom logic, that they'll otherwise be unable to implement.

If you don't want to update all clients, the usual thing to do is to add yet another flag in `CFG::BuildOptions`, and leave it off by default so that different clients could opt-in gradually as they implement support for the new element.

Given that thread-safety analysis is an analysis-based warning, and all analysis-based warnings share the same CFG, you'll probably still have to update the other analysis-based warnings. But there are like 4 of them and all their entry points are in the same function so you can easily see what they are.


================
Comment at: clang/lib/Analysis/CFG.cpp:1951-1952
+
+      SmallVector<Expr *> Args;
+      Args.push_back(DRE);
+      auto A = CallExpr::Create(*Context, F, Args, FD->getType(), VK_PRValue,
----------------
tbaeder wrote:
> NoQ wrote:
> > Shouldn't there be a unary operator `&` here somewhere? In your example the variable is `int` but the function accepts `int *`.
> Hm, I think you are right, but it doesn't make difference for the CFG (at least not for thread anysis), does it? Is there a way for me to test that I pass the right parameter value?
> it doesn't make difference for the CFG

Hmm wait no that's not how it's supposed to be. The CFG is supposed to have every sub-expression flattened. So if you're adding
```
CallExpr 'foo'
`- DeclRefExpr 'x'
```
to the CFG, you're supposed to be adding two elements: one for `DeclRefExpr`, then one for `CallExpr` *below* it:
```
[B1]
1. DeclRefExpr 'x'
2. CallExpr 'foo([B1.1])'
```
And if you also add a `UnaryOperator`:
```
CallExpr 'foo'
`- UnaryOperator '&'
   `- DeclRefExpr 'x'
```
then it's gotta be three CFG elements:
```
[B1]
1. DeclRefExpr 'x'
2. UnaryOperator '&[B1.1]'
3. CallExpr 'foo([B1.2])'
```

This is what clients expect. This probably doesn't matter for thread safety analysis because this `DeclRefExpr` doesn't have any effect on that analysis, but other clients expect these expressions to be fully flattened this way. (This is especially important in expressions with control flow such as `&&` or `?:`.)

Typically `CallExpr`s will also have a `DeclRefExpr` to the callee function as a child, probably with some function to pointer decay, which is a couple more elements.


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

https://reviews.llvm.org/D152504