[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

[Gauthier via Phabricator via cfe-commits]

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:
> > 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.


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

https://reviews.llvm.org/D59467