NoQ added inline comments.
================
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
----------------
Tyker wrote:
> 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
Hmm, i see. I got it all wrong. Please forgive me!
You're just trying to make `CFGBuilder` support `AttributedStmt` correctly in
general. And the logic you're writing says "support it as any other generic
Stmt that doesn't have any control flow in it, unless it's a
`[[likely]]`-attributed `AttributedStmt`, in which case jump straight to
children".
Could you instead do this by implementing `CFGBuilder::VisitAttributedStmt`
with that logic?
I'm also not sure this logic is actually specific to `[[likely]]`. Maybe we
should unwrap all `AttributedStmt`s similarly?
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
