[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus reopened this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Causes crashes on Sema.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365036: [analyzer][CFG] Return the correct terminator 
condition (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63538?vs=205909=207772#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63538

Files:
  cfe/trunk/include/clang/Analysis/CFG.h
  cfe/trunk/lib/Analysis/CFG.cpp

Index: cfe/trunk/lib/Analysis/CFG.cpp
===
--- cfe/trunk/lib/Analysis/CFG.cpp
+++ cfe/trunk/lib/Analysis/CFG.cpp
@@ -5615,69 +5615,21 @@
   Out << JsonFormat(TempOut.str(), AddQuotes);
 }
 
-Stmt *CFGBlock::getTerminatorCondition(bool StripParens) {
-  Stmt *Terminator = getTerminatorStmt();
-  if (!Terminator)
+const Stmt *CFGBlock::getTerminatorCondition(bool StripParens) const {
+  // If the terminator is a temporary dtor or a virtual base, etc, we can't
+  // retrieve a meaningful condition, bail out.
+  if (rbegin()->getKind() != CFGElement::Kind::Statement)
 return nullptr;
 
-  Expr *E = nullptr;
-
-  switch (Terminator->getStmtClass()) {
-default:
-  break;
-
-case Stmt::CXXForRangeStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ForStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::WhileStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::DoStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::IfStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ChooseExprClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::IndirectGotoStmtClass:
-  E = cast(Terminator)->getTarget();
-  break;
-
-case Stmt::SwitchStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::BinaryConditionalOperatorClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ConditionalOperatorClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::BinaryOperatorClass: // '&&' and '||'
-  E = cast(Terminator)->getLHS();
-  break;
-
-case Stmt::ObjCForCollectionStmtClass:
-  return Terminator;
+  // This should be the condition of the terminator block.
+  const Stmt *S = rbegin()->castAs().getStmt();
+  if (isa(S)) {
+return getTerminatorStmt();
   }
 
-  if (!StripParens)
-return E;
-
-  return E ? E->IgnoreParens() : nullptr;
+  // Only ObjCForCollectionStmt is known not to be a non-Expr terminator.
+  const Expr *Cond = cast(S);
+  return StripParens ? Cond->IgnoreParens() : Cond;
 }
 
 //===--===//
Index: cfe/trunk/include/clang/Analysis/CFG.h
===
--- cfe/trunk/include/clang/Analysis/CFG.h
+++ cfe/trunk/include/clang/Analysis/CFG.h
@@ -860,10 +860,12 @@
   Stmt *getTerminatorStmt() { return Terminator.getStmt(); }
   const Stmt *getTerminatorStmt() const { return Terminator.getStmt(); }
 
-  Stmt *getTerminatorCondition(bool StripParens = true);
+  /// \returns the condition of the terminator (condition of an if statement,
+  /// for loop, etc).
+  const Stmt *getTerminatorCondition(bool StripParens = true) const;
 
-  const Stmt *getTerminatorCondition(bool StripParens = true) const {
-return const_cast(this)->getTerminatorCondition(StripParens);
+  const Expr *getTerminatorConditionExpr(bool StripParens = true) const {
+return dyn_cast_or_null(getTerminatorCondition(StripParens));
   }
 
   const Stmt *getLoopTarget() const { return LoopTarget; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested review of this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Seems fine over here let's see what happens if I try to land it?


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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus added a comment.

Something's off. `test/Analysis/edges-new.mm` fails, but I don't remember any 
test failures at home -- I'll investigate.

Before this patch:
F9448950: image.png 
After this patch:
F9448940: image.png 


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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This change will be really useful for the lifetime analysis as well! Thanks!

I have one concern though. The analyzer is using linerarized (or threaded) CFGs 
with every subexpression being a separate entry in the basic blocks. Will your 
change give sensible answers for non-linearized CFGs? If not, this should be 
documented. Do we have users of this API with non-linearized CFGs?


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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Ok, thanks!


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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 205909.
Szelethus added a comment.

Addressing reviewer feedback!


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

https://reviews.llvm.org/D63538

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp

Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -5615,69 +5615,21 @@
   Out << JsonFormat(TempOut.str(), AddQuotes);
 }
 
-Stmt *CFGBlock::getTerminatorCondition(bool StripParens) {
-  Stmt *Terminator = getTerminatorStmt();
-  if (!Terminator)
+const Stmt *CFGBlock::getTerminatorCondition(bool StripParens) const {
+  // If the terminator is a temporary dtor or a virtual base, etc, we can't
+  // retrieve a meaningful condition, bail out.
+  if (rbegin()->getKind() != CFGElement::Kind::Statement)
 return nullptr;
 
-  Expr *E = nullptr;
-
-  switch (Terminator->getStmtClass()) {
-default:
-  break;
-
-case Stmt::CXXForRangeStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ForStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::WhileStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::DoStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::IfStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ChooseExprClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::IndirectGotoStmtClass:
-  E = cast(Terminator)->getTarget();
-  break;
-
-case Stmt::SwitchStmtClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::BinaryConditionalOperatorClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::ConditionalOperatorClass:
-  E = cast(Terminator)->getCond();
-  break;
-
-case Stmt::BinaryOperatorClass: // '&&' and '||'
-  E = cast(Terminator)->getLHS();
-  break;
-
-case Stmt::ObjCForCollectionStmtClass:
-  return Terminator;
+  // This should be the condition of the terminator block.
+  const Stmt *S = rbegin()->castAs().getStmt();
+  if (isa(S)) {
+return getTerminatorStmt();
   }
 
-  if (!StripParens)
-return E;
-
-  return E ? E->IgnoreParens() : nullptr;
+  // Only ObjCForCollectionStmt is known not to be a non-Expr terminator.
+  const Expr *Cond = cast(S);
+  return StripParens ? Cond->IgnoreParens() : Cond;
 }
 
 //===--===//
Index: clang/include/clang/Analysis/CFG.h
===
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -860,10 +860,12 @@
   Stmt *getTerminatorStmt() { return Terminator.getStmt(); }
   const Stmt *getTerminatorStmt() const { return Terminator.getStmt(); }
 
-  Stmt *getTerminatorCondition(bool StripParens = true);
+  /// \returns the condition of the terminator (condition of an if statement,
+  /// for loop, etc).
+  const Stmt *getTerminatorCondition(bool StripParens = true) const;
 
-  const Stmt *getTerminatorCondition(bool StripParens = true) const {
-return const_cast(this)->getTerminatorCondition(StripParens);
+  const Expr *getTerminatorConditionExpr(bool StripParens = true) const {
+return dyn_cast_or_null(getTerminatorCondition(StripParens));
   }
 
   const Stmt *getLoopTarget() const { return LoopTarget; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think this kinda makes sense, but also it looks like a risky change, because 
who knows how many times we have took the old behavior for granted. In 
particular, i'm slightly worried about this breaking the wonky logic of 
`ExprEngine::VisitLogicalExpr` (cf. D59857 ). 
But i've no specific concerns, so i think it's worth a try, given that it's a 
massive simplification.




Comment at: clang/lib/Analysis/CFG.cpp:5628-5630
+  const Expr *Cond;
 
+  if (!(Cond = dyn_cast(S))) {

Can we do the assignment to the previous line pls? ^.^



Comment at: clang/lib/Analysis/CFG.cpp:5631-5634
+// Only ObjCForCollectionStmt is known not to be a non-Expr terminator.
+const auto *O = cast(S);
 
+Cond = O->getCollection();

The new arrow doesn't really make much sense. Like, there's nothing interesting 
going on specifically with the collection, so there's no reason to highlight it 
separately from, say, the element we're extracting from it. Let's just keep the 
old behavior.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63538



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, a_sidorin, baloghadamsoftware, 
rnkovacs, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, Charusso, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

For the following terminator statement:

  if (A && B && C && D)

The built CFG is the following:

  [B5 (ENTRY)]
Succs (1): B4
  
  [B1]
1: 10
2: j
3: [B1.2] (ImplicitCastExpr, LValueToRValue, int)
4: [B1.1] / [B1.3]
5: int x = 10 / j;
Preds (1): B2
Succs (1): B0
  
  [B2]
1: C
2: [B2.1] (ImplicitCastExpr, LValueToRValue, _Bool)
T: if [B4.4] && [B3.2] && [B2.2]
Preds (1): B3
Succs (2): B1 B0
  
  [B3]
1: B
2: [B3.1] (ImplicitCastExpr, LValueToRValue, _Bool)
T: [B4.4] && [B3.2] && ...
Preds (1): B4
Succs (2): B2 B0
  
  [B4]
1: 0
2: int j = 0;
3: A
4: [B4.3] (ImplicitCastExpr, LValueToRValue, _Bool)
T: [B4.4] && ...
Preds (1): B5
Succs (2): B3 B0
  
  [B0 (EXIT)]
Preds (4): B1 B2 B3 B4

However, even though the path of execution in B2  
only depends on C's value, `CFGBlock::getCondition()` would return the entire 
condition (`A && B && C`). For B3 , it would 
return `A && B`. I changed this the actual condition.

The tests show an addition of an extra arrow for `ObjCForCollectionStmt`, all 
of them similar to this:
F9306419: image.png 
(the first arrow the addition)


Repository:
  rC Clang

https://reviews.llvm.org/D63538

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist

Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -5743,11 +5743,45 @@
   
 
 
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line160
+   col3
+   file0
+  
+  
+   line160
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line160
+   col18
+   file0
+  
+  
+   line160
+   col20
+   file0
+  
+ 
+   
+  
+
+
  kindevent
  location
  
   line160
-  col8
+  col18
   file0
  
  ranges
@@ -5755,12 +5789,12 @@

 
  line160
- col8
+ col18
  file0
 
 
  line160
- col13
+ col20
  file0
 

@@ -5780,12 +5814,12 @@
  
   
line160
-   col3
+   col18
file0
   
   
line160
-   col5
+   col20
file0
   
  
Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -2182,11 +2182,45 @@
   
 
 
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line131
+   col3
+   file0
+  
+  
+   line131
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line131
+   col15
+   file0
+  
+  
+   line131
+   col15
+   file0
+  
+ 
+   
+  
+
+
  kindevent
  location
  
   line131
-  col8
+  col15
   file0
  
  ranges
@@ -2194,12 +2228,12 @@

 
  line131
- col8
+ col15
  file0
 
 
  line131
- col10
+ col15
  file0
 

@@ -2219,12 +2253,12 @@
  
   
line131
-   col3
+   col15
file0
   
   
line131
-   col5
+   col15
file0
   
  
@@ -2457,11 +2491,45 @@
   
 
 
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line137
+   col3
+   file0
+  
+  
+   line137
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line137
+   col18
+   file0
+  
+  
+   line137
+   col20
+   file0
+