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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
