[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. [&a, &b, &c]), 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<ParmVarDecl>(VD);
+  const bool IsEntryValue =
+      isa<ParmVarDecl>(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<ParmVarDecl>(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