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:
> > > 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?
> we shouldn't handle all AttributedStmt this way. for example fallthrough
> needs to be present in the CFG because the analysis based warning depends on
> it. but we probably can handle every AttributedStmt on a case statement this
> way or even every AttributedStmt that aren't applied on a NullStmt. but
> perhaps this should be in an other patch, this one is already quite big and
> this issue is more general the likely attribute.
Thanks, this makes perfect sense to me!
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
