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

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua marked an inline comment as done.
li.zhe.hua 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) {
----------------
ymandel wrote:
> 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.
> I'm afraid this may allow us to hide a bug.

Ah, yes, that is definitely true.

> That said, any idea if we actually need to use `Reference` skipping at all?

Whatever we choose, I imagine it should be kept the same as the analogous line in [[ https://github.com/llvm/llvm-project/blob/118c5d1c97b4191678663bf2a938eee7dec6f0b1/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L112-L113 | extendFlowCondition ]]? I've opted to not change it for now, given my uncertainty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125821