Tyker marked an inline comment as done.
Tyker added inline comments.
================
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
----------------
NoQ wrote:
> Tyker wrote:
> > riccibruno wrote:
> > > I don't understand why this is needed. Can you explain it ? Also I think
> > > that someone familiar with this code should comment on this (maybe @NoQ ?)
> > the detail of why are complicated and i don't have them all in head but
> > without this edit in cases like
> >
> > ```
> > switch (...) {
> > [[likely]] case 1:
> > ...
> > [[fallthrough]];
> > default:
> > ...
> > }
> > ```
> > the fallthrough attribute emitted a diagnostic because is wasn't handling
> > attributed case statement. the edit i performed is probably not the optimal
> > way to solve the issue as it only solves the issue for likelihood
> > attribute. but i don't know any other attribute that can be applied on a
> > case statement but if they were others they would probably have the same
> > issue. but the code is quite hard to follow and i didn't wanted to break
> > anything. so this is what i came up with.
> > i am going to look into it to find a better solution.
> The [[likely]] attribute should not affect the overall topology of the CFG.
> It might be a nice piece of metadata to add to a CFG edge or to a CFG
> terminator, but for most consumers of the CFG (various static analyses such
> as analysis-based warnings or the Static Analyzer) the attribute should have
> little to no effect - the tool would still need to explore both branches. I
> don't know how exactly the fallthrough warning operates, but i find it likely
> (no pun intended) that the fallthrough warning itself should be updated, not
> the CFG.
>
> It is probable that for compiler warnings it'll only cause false negatives,
> which is not as bad as false positives, but i wouldn't rely on that.
> Additionally, false negatives in such rare scenarios will be very hard to
> notice later. So i'm highly in favor of aiming for the correct solution in
> this patch.
>
>
i think we all agree that the CFG structure shouldn't be affected by the
presence or absence of the likely attribute. but in the current state(no
changes to the CFG) it does for example.
the CFG were obtained without the edit in CFG.cpp but with the added likely
attribute
using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
input:
```
int f(int i) {
switch (i) {
[[likely]] case 1:
return 1;
}
return i;
}
```
outputs:
```
[B5 (ENTRY)]
Succs (1): B2
[B1]
1: i
2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
3: return [B1.2];
Preds (2): B3 B2
Succs (1): B0
[B2]
1: i
2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
T: switch [B2.2]
Preds (1): B5
Succs (2): B4 B1
[B3]
1: [[likely]]case 1:
[B4.2] Succs (1): B1
[B4]
case 1:
1: 1
2: return [B4.1];
Preds (1): B2
Succs (1): B0
[B0 (EXIT)]
Preds (2): B1 B4
```
and
input:
```
int f(int i) {
switch (i) {
case 1:
return 1;
}
return i;
}
```
outputs:
```
[B4 (ENTRY)]
Succs (1): B2
[B1]
1: i
2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
3: return [B1.2];
Preds (1): B2
Succs (1): B0
[B2]
1: i
2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
T: switch [B2.2]
Preds (1): B4
Succs (2): B3 B1
[B3]
case 1:
1: 1
2: return [B3.1];
Preds (1): B2
Succs (1): B0
[B0 (EXIT)]
Preds (2): B1 B3
```
i think think this is the underlying issue. the false diagnostic from
fallthrough previously mentioned is a consequence of this
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
