[analyzer] LoopUnrolling: fix crash when a parameter is a loop counter

[AbbasSabra]

Summary

When loop counter is a function parameter "isPossiblyEscaped" will not find the variable declaration "VD" which lead to "llvm_unreachable". Parameters should be escaped like global variables to avoid a crash.

[xazax.hun]

Thanks for finding this bug! Adding some reviewers.

I think it would be perfectly fine to unroll loops where the loop counter is a pass-by-value parameter. That should not be considered escaped.

I think in case of parameters isPossiblyEscaped should return false for values and true for references.

What do you think?

[xazax.hun]

clang/test/Analysis/loop-unrolling.cpp

503

nit: we usually use arg for actual arguments at the call site, and use param for formal parameters. The test function should be renamed accordingly. Also, it would be great to add clang_analyzer_numTimesReached() to demonstrate whether the loop is actually unrolled or not.

[vsavchenko]

Thanks for taking your time to find and resolve the bug.
I agree that this solution seems a bit too harsh. Technically there is no difference between local variable declaration and a parameter declaration (if we are talking about value types as pointed out by @xazax.hun).
I believe that we should actually fix the will not find the variable declaration "VD" which lead to "llvm_unreachable" part.

[AbbasSabra]

Fix code review

[AbbasSabra]

update

[AbbasSabra]

What you both said makes sense. Now, reference parameters are escaped and value parameters are treated as the local variables.
Thanks!

[vsavchenko]

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp

172

getKind function is only an implementation detail for isa/cast/dan_cast functions (docs). So, this condition would be better in the following form: isa<ParmVarDecl>(VD).

NOTE: One good motivation here is that maybe in the future there will be some sort of new type of function parameters and it will be derived from ParmVarDecl. In this situation, direct comparison with the kind of node will not work and probably won't be fixed by developers who introduced the new node, but isa approach will stay correct.

[AbbasSabra]

Fix code review 2

[AbbasSabra]

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp

172

Thanks for the detailed explanation.

[NoQ]

Looks great, thanks!

This is probably not the precise solution (i.e., we might be able to see where exactly is the reference taken and proceed from there) but neither is the original code - it's always been super pattern-matchy,

[vsavchenko]

LGTM! Thanks again!

[AbbasSabra]

Great! Can someone take care of merging it? I believe I don't have access.

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp

return forStmt(
                  anyOf(hasOperatorName("++"), hasOperatorName("--")),
                  hasUnaryOperand(declRefExpr(
                      to(varDecl(allOf(equalsBoundNode("initVarName"),
                                       hasType(isInteger())))))))),
              unless(hasBody(hasSuspiciousStmt("initVarName")))).bind("forLoop");
 }
 
 static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
-  // Global variables and parameters assumed as escaped variables.
-  if (VD->hasGlobalStorage() || VD->getKind() == Decl::ParmVar)
+  // Global variables assumed as escaped variables.
+  if (VD->hasGlobalStorage())
+    return true;
+
+  const bool isParm = VD->getKind() == Decl::ParmVar;
+  // Reference parameters are assumed as escaped variables.
+  if (isParm && VD->getType()->isReferenceType())
     return true;
 
   while (!N->pred_empty()) {
     // FIXME: getStmtForDiagnostics() does nasty things in order to provide
     // a valid statement for body farms, do we need this behavior here?
     const Stmt *S = N->getStmtForDiagnostics();
     if (!S) {
       N = N->getFirstPred();
       continue;
     }
 
auto Match =
         match(stmt(anyOf(callByRef(equalsNode(VD)), getAddrTo(equalsNode(VD)),
                          assignedToRef(equalsNode(VD)))),
               *S, ASTCtx);
     if (!Match.empty())
       return true;
 
     N = N->getFirstPred();
   }
+
+  // Parameter declaration will not be found.
+  if (isParm)
+    return false;
+
   llvm_unreachable("Reached root without finding the declaration of VD");
 }
 
 bool shouldCompletelyUnroll(const Stmt *LoopStmt, ASTContext &ASTCtx,
                             ExplodedNode *Pred, unsigned &maxStep) {
 
   if (!isLoopStmt(LoopStmt))
     return false;

clang/test/Analysis/loop-unrolling.cpp

 
 
 void pr34943() {
   for (int i = 0; i < 6L; ++i) {
     clang_analyzer_numTimesReached(); // expected-warning {{6}}
   }
 }
 
-void arg_as_loop_counter(int i) {
+void parm_by_value_as_loop_counter(int i) {
   for (i = 0; i < 10; ++i) {
-    (void)i;
+    clang_analyzer_numTimesReached(); // expected-warning {{10}}
+  }
+}
+
+void parm_by_ref_as_loop_counter(int &i) {
+  for (i = 0; i < 10; ++i) {
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }