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

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 1 18:22:31 PDT 2019


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





More information about the cfe-commits mailing list