[PATCH] D125821: [clang][dataflow] Fix double visitation of nested logical operators

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:574
 
-    // Sub-expressions that are logic operators are not added in basic blocks
-    // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
-    // operator, it isn't evaluated and assigned a value yet. In that case, we
-    // need to first visit `SubExpr` and then try to get the value that gets
-    // assigned to it.
-    Visit(&SubExpr);
-    if (auto *Val = dyn_cast_or_null<BoolValue>(
-            Env.getValue(SubExpr, SkipPast::Reference)))
+    auto *SubExprLoc = Env.getStorageLocation(SubExpr, SkipPast::Reference);
+    if (SubExprLoc == nullptr) {
----------------
I'm afraid this may allow us to hide a bug.  Specifically: consider if `SubExpr` was already evaluated to a RefenceValue and that value's StorageLocation does *not* map to a value. Then, this line will be true, but we won't want to revisit the `SubExrp`. Now, that may be impossible in practice, so I'm not sure how big a problem this is, but it seems safer to just use `SkipPast::None` and not rely on any guarantees.

That said, i see why  you want the uniformity of `Reference` because it is used below. That said, any idea if we actually need to use `Reference` skipping at all? I think so -- the case of a variable or field access as sub-expression, but those probably have a cast around them anyhow, so i'm not sure those will require reference skipping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125821