[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1af97c9d0b02: [analyzer] LoopUnrolling: fix crash when a 
loop counter is captured in a lambda… (authored by AbbasSabra, committed by 
vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,52 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
-  // Global variables assumed as escaped variables.
+static bool isCapturedByReference(ExplodedNode *N, const DeclRefExpr *DR) {
+
+  // Get the 

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

In D102273#2866532 , @vsavchenko 
wrote:

> Great!  Thanks for addressing all of the comments!

Thank you for the review! Can you take care of merging it? I don't have the 
required permission.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Great!  Thanks for addressing all of the comments!




Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

AbbasSabra wrote:
> vsavchenko wrote:
> > AbbasSabra wrote:
> > > vsavchenko wrote:
> > > > But actually, it's a bit different with this one. I don't know exact 
> > > > details and rules how clang sema fills in the class for lambda.
> > > > According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
> > > > cppreference ]]:
> > > > > For the entities that are captured by reference (with the default 
> > > > > capture [&] or when using the character &, e.g. [, , ]), it is 
> > > > > unspecified if additional data members are declared in the closure 
> > > > > type
> > > > 
> > > > It can be pretty much specified in clang, that's true, but it looks 
> > > > like in `DeadStoreChecker` we have a very similar situation and we do 
> > > > not assume that captured variable always have a corresponding field.
> > > Yes, I based this on the fact that DeadStoreChecker considers it 
> > > possible, but I have never faced a case where it does not have a 
> > > corresponding field.
> > It still would be good to have some proof that it is indeed like this or 
> > simply fallback into returning true (which you already do when in doubt).
> As I said, I believe it cannot happen but I assumed based on the other 
> checker that there is something I don't know.
> I think based on !!getCaptureFields!! the implementation we are iterating 
> over all captures. For that algorithm to work number of captures should be 
> equal to number of fields
> ```
> void CXXRecordDecl::getCaptureFields(
>llvm::DenseMap ,
>FieldDecl *) const {
>   Captures.clear();
>   ThisCapture = nullptr;
> 
>   LambdaDefinitionData  = getLambdaData();
>   RecordDecl::field_iterator Field = field_begin();
>   for (const LambdaCapture *C = Lambda.Captures, *CEnd = C + 
> Lambda.NumCaptures;
>C != CEnd; ++C, ++Field) {
> if (C->capturesThis())
>   ThisCapture = *Field;
> else if (C->capturesVariable())
>   Captures[C->getCapturedVar()] = *Field;
>   }
>   assert(Field == field_end());
> }
> ```
> 
> I dropped the defensive code
OK, sounds reasonable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

@vsavchenko any update on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

Note: I don't have the right to re-run the failed build/test. I assume that it 
is not related to my change since the test compile and runs omp code(Static 
analyzer doesn't run on that test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.
Herald added a subscriber: manas.

In D102273#2766531 , @NoQ wrote:

> I've just been patching up clang-tidy's infinite loop checker and the problem 
> sounds s similar. Maybe we should move clang-tidy's alias analysis into 
> `libAnalysis` and re-use it?

I'm not familiar with clang-tidy's alias analysis. Do you think it should be 
part of this patch? or a follow-up one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I've just been patching up clang-tidy's infinite loop checker and the problem 
sounds s similar. Maybe we should move clang-tidy's alias analysis into 
`libAnalysis` and re-use it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra updated this revision to Diff 345482.
AbbasSabra added a comment.

Updating D102273 : [analyzer] Update comments 
+ fix typos


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,52 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
-  // Global variables assumed as escaped variables.
+static bool isCapturedByReference(ExplodedNode *N, const DeclRefExpr *DR) {
+
+  // Get the lambda CXXRecordDecl
+  assert(DR->refersToEnclosingVariableOrCapture());
+  const LocationContext *LocCtxt = 

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

vsavchenko wrote:
> AbbasSabra wrote:
> > vsavchenko wrote:
> > > But actually, it's a bit different with this one. I don't know exact 
> > > details and rules how clang sema fills in the class for lambda.
> > > According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
> > > cppreference ]]:
> > > > For the entities that are captured by reference (with the default 
> > > > capture [&] or when using the character &, e.g. [, , ]), it is 
> > > > unspecified if additional data members are declared in the closure type
> > > 
> > > It can be pretty much specified in clang, that's true, but it looks like 
> > > in `DeadStoreChecker` we have a very similar situation and we do not 
> > > assume that captured variable always have a corresponding field.
> > Yes, I based this on the fact that DeadStoreChecker considers it possible, 
> > but I have never faced a case where it does not have a corresponding field.
> It still would be good to have some proof that it is indeed like this or 
> simply fallback into returning true (which you already do when in doubt).
As I said, I believe it cannot happen but I assumed based on the other checker 
that there is something I don't know.
I think based on !!getCaptureFields!! the implementation we are iterating over 
all captures. For that algorithm to work number of captures should be equal to 
number of fields
```
void CXXRecordDecl::getCaptureFields(
   llvm::DenseMap ,
   FieldDecl *) const {
  Captures.clear();
  ThisCapture = nullptr;

  LambdaDefinitionData  = getLambdaData();
  RecordDecl::field_iterator Field = field_begin();
  for (const LambdaCapture *C = Lambda.Captures, *CEnd = C + Lambda.NumCaptures;
   C != CEnd; ++C, ++Field) {
if (C->capturesThis())
  ThisCapture = *Field;
else if (C->capturesVariable())
  Captures[C->getCapturedVar()] = *Field;
  }
  assert(Field == field_end());
}
```

I dropped the defensive code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra updated this revision to Diff 345471.
AbbasSabra marked 7 inline comments as done.
AbbasSabra added a comment.

Updating D102273 : [analyzer] Apply code 
review part 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,54 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
-  // Global variables assumed as escaped variables.
+static bool isCapturedByReference(ExplodedNode *N, const DeclRefExpr *DR) {
+
+  // Get the lambda CXXRecordDecl
+  assert(DR->refersToEnclosingVariableOrCapture());
+  const 

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

AbbasSabra wrote:
> vsavchenko wrote:
> > But actually, it's a bit different with this one. I don't know exact 
> > details and rules how clang sema fills in the class for lambda.
> > According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
> > cppreference ]]:
> > > For the entities that are captured by reference (with the default capture 
> > > [&] or when using the character &, e.g. [, , ]), it is unspecified 
> > > if additional data members are declared in the closure type
> > 
> > It can be pretty much specified in clang, that's true, but it looks like in 
> > `DeadStoreChecker` we have a very similar situation and we do not assume 
> > that captured variable always have a corresponding field.
> Yes, I based this on the fact that DeadStoreChecker considers it possible, 
> but I have never faced a case where it does not have a corresponding field.
It still would be good to have some proof that it is indeed like this or simply 
fallback into returning true (which you already do when in doubt).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Thanks for addressing my comments! I still have some left though




Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:171
+  const Decl *D = LocCtxt->getDecl();
+  const auto *MD = dyn_cast_or_null(D);
+  assert(MD && MD->getParent()->isLambda() &&

`dyn_cast_or_null` also tells the reader that `D` can be null and can be not 
`CXXMethodDecl`.  So it's better to use `cast` here if you are sure about `D` 
and have assertions about it.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:175-178
+  const CXXRecordDecl *CXXRec = MD->getParent();
+  llvm::DenseMap LambdaCaptureFields;
+  FieldDecl *LambdaThisCaptureField;
+  CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);

Body of this function seems to be too dense.  It is usually hard to read 
something that doesn't have rest places for eyes.  You can think of it as 
paragraphs in the written text.
In this particular situation, you can split all these statements and 
declarations in groups of 3-4.  It would be perfect if they are grouped 
semantically (like good paragraphs).



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:179
+  CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
+  const VarDecl *VD = dyn_cast(DR->getDecl()->getCanonicalDecl());
+  assert(VD);

We can just do `cast` here



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+return FD->getType()->isReferenceType();
+  } else {
+assert(false && "Unknown captured variable");
+  }

I still think that we should do something about this `else`



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:192-193
+
+// A loop counter is considered escaped if:
+// - It is a global variable.
+// - It is an entry value: a reference parameter or a reference capture.

I think it's a great idea to list all the cases.  I think we can go even 
further: enumerate them and actually pinpoint in the implementation where we 
cover which case.  With numbers it can be much easier.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:194
+// - It is a global variable.
+// - It is an entry value: a reference parameter or a reference capture.
+// - It is assigned to a non-const reference variable or parameter.

Can we not introduce new terms and simply call it a **reference**?



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:197
+// - Has its address taken.
+static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N,
+  const DeclRefExpr *DR) {

This should be also modified then


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

vsavchenko wrote:
> But actually, it's a bit different with this one. I don't know exact details 
> and rules how clang sema fills in the class for lambda.
> According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
> cppreference ]]:
> > For the entities that are captured by reference (with the default capture 
> > [&] or when using the character &, e.g. [, , ]), it is unspecified if 
> > additional data members are declared in the closure type
> 
> It can be pretty much specified in clang, that's true, but it looks like in 
> `DeadStoreChecker` we have a very similar situation and we do not assume that 
> captured variable always have a corresponding field.
Yes, I based this on the fact that DeadStoreChecker considers it possible, but 
I have never faced a case where it does not have a corresponding field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra updated this revision to Diff 345435.
AbbasSabra marked 4 inline comments as done.
AbbasSabra added a comment.

Updating D102273 : [analyzer] Apply code 
review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,53 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
+static bool isCapturedByReference(ExplodedNode *N, const DeclRefExpr *DR) {
+  assert(DR->refersToEnclosingVariableOrCapture());
+
+  const LocationContext *LocCtxt = N->getLocationContext();
+  const Decl *D = LocCtxt->getDecl();
+  

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

By checking the line coverage of the `LoopUnrolling.cpp` test file, looks like 
all lines are covered you touched.

There are only two return statements uncovered though: L200, L251.
We should consider extending this test file to cover them as well in a 
follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Great job on the patch! Thanks!




Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:166-167
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
+static bool isCapturedByReference(const VarDecl *VD, ExplodedNode *N,
+  const DeclRefExpr *DR) {
+  assert(DR->refersToEnclosingVariableOrCapture());

One thought here.
I think it's better to not have implicit connections between parameters that 
are not obvious to the user of the function (or to the person who is going to 
modify this function in the future).  Here we always have `VD = 
DR->getDecl()->getCanonicalDecl()`. So, IMO, the interface of this function 
will be much cleaner if we accept only a `DeclRefExpr`.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

But actually, it's a bit different with this one. I don't know exact details 
and rules how clang sema fills in the class for lambda.
According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
cppreference ]]:
> For the entities that are captured by reference (with the default capture [&] 
> or when using the character &, e.g. [, , ]), it is unspecified if 
> additional data members are declared in the closure type

It can be pretty much specified in clang, that's true, but it looks like in 
`DeadStoreChecker` we have a very similar situation and we do not assume that 
captured variable always have a corresponding field.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:188-191
+  } else {
+assert(false &&
+   "Captured variable should only be seen while evaluating a lambda");
+  }

There is a general rule in LLVM's style guide of trying to minimize nestedness 
of `if's, `forms and so on.
In this particular situation this assertion can be more local: `assert(MD && 
MD->getParent()->isLambda() & ...);`.
This way it is more obvious what it checks and we avoid confusion while reading 
this function because `if (condition)` automatically means that `!condition` is 
something that might happen.
The same applies to another assertion here.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:202-203
 
-  const bool isParm = isa(VD);
+  const bool IsEntryValue =
+  isa(VD) || DR->refersToEnclosingVariableOrCapture();
   // Reference parameters are assumed as escaped variables.

I think this whole section is less obvious now and requires one good solid 
comment explaining what are the different situations here, what is "entry" and 
so on.



Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:204
+  isa(VD) || DR->refersToEnclosingVariableOrCapture();
   // Reference parameters are assumed as escaped variables.
+  if ((DR->refersToEnclosingVariableOrCapture() &&

I think that this comment has to be updated now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Balázs Benics via Phabricator via cfe-commits]

steakhal added reviewers: NoQ, vsavchenko, steakhal.
steakhal added a comment.

I'm gonna have a look at this tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra created this revision.
Herald added subscribers: steakhal, ASDenysPetrov, martong, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, zzheng, szepet, baloghadamsoftware, 
xazax.hun.
AbbasSabra requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,52 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
+static bool isCapturedByReference(const VarDecl *VD, ExplodedNode *N,
+  const DeclRefExpr *DR) {
+  assert(DR->refersToEnclosingVariableOrCapture());
+
+