[clang] [NFC][analyzer] Improve computeObjectUnderConstruction (PR #186186)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/186186 at github.com>


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

<details>
<summary>Changes</summary>

Previously the method `ExprEngine::computeObjectUnderConstruction` took a `NodeBuilderContext` parameter which was only used to call its `blockCount()` method; this commit replaces this with directly taking `NumVisitedCaller` (= number of times the caller was visited, the `blockCount`) as an `unsigned` value.

Also leave a FIXME comment on some logic that seems to be fishy (I _suspect_ that sometimes it uses the wrong `LocationContext` and `Block`, but I'm not certain.)

---
Full diff: https://github.com/llvm/llvm-project/pull/186186.diff


3 Files Affected:

- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (+3-3) 
- (modified) clang/lib/StaticAnalyzer/Core/CallEvent.cpp (+5-1) 
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+6-8) 


``````````diff
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 2023a7a5b1ac8..360a260d4d803 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -796,7 +796,7 @@ class ExprEngine {
   /// A multi-dimensional array is also a continuous memory location in a
   /// row major order, so for arr[0][0] Idx is 0 and for arr[3][3] Idx is 8.
   SVal computeObjectUnderConstruction(const Expr *E, ProgramStateRef State,
-                                      const NodeBuilderContext *BldrCtx,
+                                      unsigned NumVisitedCaller,
                                       const LocationContext *LCtx,
                                       const ConstructionContext *CC,
                                       EvalCallOptions &CallOpts,
@@ -818,8 +818,8 @@ class ExprEngine {
       const LocationContext *LCtx, const ConstructionContext *CC,
       EvalCallOptions &CallOpts, unsigned Idx = 0) {
 
-    SVal V = computeObjectUnderConstruction(E, State, BldrCtx, LCtx, CC,
-                                            CallOpts, Idx);
+    SVal V = computeObjectUnderConstruction(E, State, BldrCtx->blockCount(),
+                                            LCtx, CC, CallOpts, Idx);
     State = updateObjectsUnderConstruction(V, E, State, LCtx, CC, CallOpts);
 
     return std::make_pair(State, V);
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index f6c3a8e3266da..86ffd92cdf6f5 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -562,8 +562,12 @@ std::optional<SVal> CallEvent::getReturnValueUnderConstruction() const {
 
   EvalCallOptions CallOpts;
   ExprEngine &Engine = getState()->getStateManager().getOwningEngine();
+  // FIXME: This code assumes that the _current_ location context and block is
+  // the location and block where this `CallExpr` is called. For a more stable
+  // solution `Engine.getNumVisitedCurrent()` should be replaced with a call to
+  // `Engine.getNumVisited(<CallerLCtx>, <CallerBlock>)`.
   SVal RetVal = Engine.computeObjectUnderConstruction(
-      getOriginExpr(), getState(), &Engine.getBuilderContext(),
+      getOriginExpr(), getState(), Engine.getNumVisitedCurrent(),
       getLocationContext(), CC, CallOpts);
   return RetVal;
 }
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 0866dda766667..cf22b62225f2f 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -127,9 +127,8 @@ SVal ExprEngine::makeElementRegion(ProgramStateRef State, SVal LValue,
 // In case when the prvalue is returned from the function (kind is one of
 // SimpleReturnedValueKind, CXX17ElidedCopyReturnedValueKind), then
 // it's materialization happens in context of the caller.
-// We pass BldrCtx explicitly, as currBldrCtx always refers to callee's context.
 SVal ExprEngine::computeObjectUnderConstruction(
-    const Expr *E, ProgramStateRef State, const NodeBuilderContext *BldrCtx,
+    const Expr *E, ProgramStateRef State, unsigned NumVisitedCaller,
     const LocationContext *LCtx, const ConstructionContext *CC,
     EvalCallOptions &CallOpts, unsigned Idx) {
 
@@ -230,10 +229,9 @@ SVal ExprEngine::computeObjectUnderConstruction(
           assert(!isa<BlockInvocationContext>(CallerLCtx));
         }
 
-        NodeBuilderContext CallerBldrCtx(getCoreEngine(),
-                                         SFC->getCallSiteBlock(), CallerLCtx);
+        unsigned NVCaller = getNumVisited(CallerLCtx, SFC->getCallSiteBlock());
         return computeObjectUnderConstruction(
-            cast<Expr>(SFC->getCallSite()), State, &CallerBldrCtx, CallerLCtx,
+            cast<Expr>(SFC->getCallSite()), State, NVCaller, CallerLCtx,
             RTC->getConstructionContext(), CallOpts);
       } else {
         // We are on the top frame of the analysis. We do not know where is the
@@ -273,7 +271,7 @@ SVal ExprEngine::computeObjectUnderConstruction(
       EvalCallOptions PreElideCallOpts = CallOpts;
 
       SVal V = computeObjectUnderConstruction(
-          TCC->getConstructorAfterElision(), State, BldrCtx, LCtx,
+          TCC->getConstructorAfterElision(), State, NumVisitedCaller, LCtx,
           TCC->getConstructionContextAfterElision(), CallOpts);
 
       // FIXME: This definition of "copy elision has not failed" is unreliable.
@@ -346,7 +344,7 @@ SVal ExprEngine::computeObjectUnderConstruction(
       CallEventManager &CEMgr = getStateManager().getCallEventManager();
       auto getArgLoc = [&](CallEventRef<> Caller) -> std::optional<SVal> {
         const LocationContext *FutureSFC =
-            Caller->getCalleeStackFrame(BldrCtx->blockCount());
+            Caller->getCalleeStackFrame(NumVisitedCaller);
         // Return early if we are unable to reliably foresee
         // the future stack frame.
         if (!FutureSFC)
@@ -365,7 +363,7 @@ SVal ExprEngine::computeObjectUnderConstruction(
         // because this-argument is implemented as a normal argument in
         // operator call expressions but not in operator declarations.
         const TypedValueRegion *TVR = Caller->getParameterLocation(
-            *Caller->getAdjustedParameterIndex(Idx), BldrCtx->blockCount());
+            *Caller->getAdjustedParameterIndex(Idx), NumVisitedCaller);
         if (!TVR)
           return std::nullopt;
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/186186