[analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.

[NoQ]

Summary

Despite the effort to eliminate the need for skipRValueSubobjectAdjustments(), we still encounter ASTs that require it from time to time, for example in https://bugs.llvm.org/show_bug.cgi?id=37578

One way of properly modeling such expressions would be to include the member-expression "adjustment" into the construction context of the temporary, but that's not something i'm planning to do, because such ASTs are rare and seem to be only becoming more rare over time, so for now i'm just fixing the old code.

The root cause of the problem in this example is that while evaluating the MemberExpr in

`-MemberExpr 0x7fd5ef0035b8 <col:9, col:13> 'a::(anonymous struct at test.cpp:2:3)' . 0x7fd5ee06a068
  `-CXXTemporaryObjectExpr 0x7fd5ef001f70 <col:9, col:11> 'a' 'void () noexcept' zeroing

there's no way for createTemporaryRegionIfNeeded() to communicate the newly created temporary region through the Environment (as it usually does), because all expressions so far have been prvalues.

The current code works around that problem by binding the region to the CXXTemporaryObjectExpr, which is of course a bad thing to do because we should not bind Locs to prvalue expressions, and it leads to a crash when eventually this bad binding propagates to the Store and the Store is unable to load it.

The solution is to bind the correct [lazy compound] value to the CXXTemporaryObjectExpr and then communicate the region to the caller directly via an out-parameter.

[NoQ]

Add comments for optional arguments.

The reason why i chose an out-parameter rather than returning a std::pair is because most callers presumably don't need this feature.

[NoQ]

which is of course a bad thing to do because we should not bind Locs to prvalue expressions

... of non-pointer type. On the other hand, binding NonLocs to glvalue expressions is always a bad thing to do; but, for the reference, adding this as an assertion currently crashes 92 tests.

[NoQ]

...and adding the aforementioned assertion for prvalues increases the number of crashes on tests to 196. It seems that the analyzer assigns values of improper loc-less very often, but then immediately overwrites them :/ I wonder how much performance could be gained by fixing these bugs.

[george.karpenkov]

looks good apart from minor nits

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

744 ↗

(On Diff #161073)

In general I would say that pair is still preferable, but I agree that with pre-c++17 codebase it gets too verbose.

lib/StaticAnalyzer/Core/ExprEngine.cpp

423 ↗

(On Diff #161073)

could we have braces here? Once we have "else" I would say we should have those.

2543 ↗

(On Diff #161073)

Let's initialize that to nullptr

[NoQ]

Address comments.

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

void performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
                           const CallEvent &Call);
 
   /// If the value of the given expression \p InitWithAdjustments is a NonLoc,
   /// copy it into a new temporary object region, and replace the value of the
   /// expression with that.
   ///
   /// If \p Result is provided, the new region will be bound to this expression
   /// instead of \p InitWithAdjustments.
-  ProgramStateRef createTemporaryRegionIfNeeded(ProgramStateRef State,
-                                                const LocationContext *LC,
-                                                const Expr *InitWithAdjustments,
-                                                const Expr *Result = nullptr);
+  ///
+  /// Returns the temporary region with adjustments into the optional
+  /// OutRegionWithAdjustments out-parameter if a new region was indeed needed,
+  /// otherwise sets it to nullptr.
+  ProgramStateRef createTemporaryRegionIfNeeded(
+      ProgramStateRef State, const LocationContext *LC,
+      const Expr *InitWithAdjustments, const Expr *Result = nullptr,
+      const SubRegion **OutRegionWithAdjustments = nullptr);
 
   /// Returns a region representing the first element of a (possibly
   /// multi-dimensional) array, for the purposes of element construction or
   /// destruction.
   ///
   /// On return, \p Ty will be set to the base type of the array.
   ///
   /// If the type is not an array type at all, the original value is returned.

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (!MD->isStatic()) {
         }
       }
     }
   }
 
   return state;
 }
 
-ProgramStateRef
-ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
-                                          const LocationContext *LC,
-                                          const Expr *InitWithAdjustments,
-                                          const Expr *Result) {
+ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded(
+    ProgramStateRef State, const LocationContext *LC,
+    const Expr *InitWithAdjustments, const Expr *Result,
+    const SubRegion **OutRegionWithAdjustments) {
   // FIXME: This function is a hack that works around the quirky AST
   // we're often having with respect to C++ temporaries. If only we modelled
   // the actual execution order of statements properly in the CFG,
   // all the hassle with adjustments would not be necessary,
   // and perhaps the whole function would be removed.
   SVal InitValWithAdjustments = State->getSVal(InitWithAdjustments, LC);
   if (!Result) {
     // If we don't have an explicit result expression, we're in "if needed"
     // mode. Only create a region if the current value is a NonLoc.
-    if (!InitValWithAdjustments.getAs<NonLoc>())
+    if (!InitValWithAdjustments.getAs<NonLoc>()) {
+      if (OutRegionWithAdjustments)
+        *OutRegionWithAdjustments = nullptr;
       return State;
+    }
     Result = InitWithAdjustments;
   } else {
     // We need to create a region no matter what. For sanity, make sure we don't
     // try to stuff a Loc into a non-pointer temporary region.
     assert(!InitValWithAdjustments.getAs<Loc>() ||
            Loc::isLocType(Result->getType()) ||
            Result->getType()->isMemberPointerType());
   }
State =
         State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
   } else {
     State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
   }
 
   // The result expression would now point to the correct sub-region of the
   // newly created temporary region. Do this last in order to getSVal of Init
   // correctly in case (Result == Init).
+  if (Result->isGLValue()) {
     State = State->BindExpr(Result, LC, Reg);
+  } else {
+    State = State->BindExpr(Result, LC, InitValWithAdjustments);
+  }
 
   // Notify checkers once for two bindLoc()s.
   State = processRegionChange(State, TR, LC);
 
+  if (OutRegionWithAdjustments)
+    *OutRegionWithAdjustments = cast<SubRegion>(Reg.getAsRegion());
   return State;
 }
 
 ProgramStateRef
 ExprEngine::addObjectUnderConstruction(ProgramStateRef State,
                                        const ConstructionContextItem &Item,
                                        const LocationContext *LC, SVal V) {
   ConstructedObjectKey Key(Item, LC->getStackFrame());
for (const auto I : CheckedSet) {
         SVal MDVal = svalBuilder.getFunctionPointer(MD);
         state = state->BindExpr(M, LCtx, MDVal);
 
         Bldr.generateNode(M, I, state);
         continue;
       }
 
       // Handle regular struct fields / member variables.
-      state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
-      SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+      const SubRegion *MR = nullptr;
+      state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr,
+                                            /*Result=*/nullptr,
+                                            /*OutRegionWithAdjustments=*/&MR);
+      SVal baseExprVal =
+          MR ? loc::MemRegionVal(MR) : state->getSVal(BaseExpr, LCtx);
 
       const auto *field = cast<FieldDecl>(Member);
       SVal L = state->getLValue(field, baseExprVal);
 
       if (M->isGLValue() || M->getType()->isArrayType()) {
         // We special-case rvalues of array type because the analyzer cannot
         // reason about them, since we expect all regions to be wrapped in Locs.
         // We instead treat these as lvalues and assume that they will decay to

cfe/trunk/test/Analysis/temporaries.cpp

void run() {
   t->foo(C()); // no-crash // expected-warning{{Called C++ object pointer is uninitialized}}
 }
 
 // This is after run() because the test is about picking the correct decl
 // for the parameter region, which should belong to the correct function decl,
 // and the non-definition decl should be found by direct lookup.
 void T::foo(C) {}
 } // namespace argument_virtual_decl_lookup
+
+namespace union_indirect_field_crash {
+union U {
+  struct {
+    int x;
+  };
+};
+
+template <typename T> class C {
+public:
+  void foo() const {
+    (void)(true ? U().x : 0);
+  }
+};
+
+void test() {
+  C<int> c;
+  c.foo();
+}
+} // namespace union_indirect_field_crash