[Analyzer] Fix for `ExprEngine::computeObjectUnderConstruction()` for base and delegating consturctor initializers

[baloghadamsoftware]

Summary

For /C++/ constructor initializers ExprEngine:computeUnderConstruction() asserts that they are all member initializers. This is not neccessarily true when this function is used to get the return value for the construction context thus attempts to fetch return values of base and delegating constructor initializers result in assertions. This small patch fixes this issue.

[baloghadamsoftware]

@NoQ Could you please take a look on this one? It is a fix of my earlier work.

[Szelethus]

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

[baloghadamsoftware]

In D85351#2215547, @Szelethus wrote:

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Why should we? This is just a fix for cases not covered, but it is the same functionality (retrieving the return value under construction). I added the missed cases to the test of this exact functionality.

[Szelethus]

In D85351#2247037, @baloghadamsoftware wrote:

In D85351#2215547, @Szelethus wrote:

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Why should we? This is just a fix for cases not covered, but it is the same functionality (retrieving the return value under construction). I added the missed cases to the test of this exact functionality.

I think its a bad experience if you break something while developing. Instead of getting a test failure for "delegating constructor initializers", you'll have to deal with a test that handles a variety of things at once, and are forced to tease it apart to find what just broke. When the introduced assert fires, this wouldn't be an issue, but in any non-crashing case it might be.

[baloghadamsoftware]

In D85351#2247095, @Szelethus wrote:

I think its a bad experience if you break something while developing. Instead of getting a test failure for "delegating constructor initializers", you'll have to deal with a test that handles a variety of things at once, and are forced to tease it apart to find what just broke. When the introduced assert fires, this wouldn't be an issue, but in any non-crashing case it might be.

This is a crashing case.

[baloghadamsoftware]

@NoQ could you please take a look on this short fix?

[baloghadamsoftware]

Tests separated.

[Szelethus]

The tests look great, thanks! I still lack the confidence to accept, unfortunately.

[NoQ]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

138–139

For base initializer you probably want the base class region. It may have a non-trivial offset and it also has the correct type and extent.

[baloghadamsoftware]

Fix for base constructors, test extended.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

138–139

Thank you for noticing this! You are completely right! Now I extended the tests with comparing the type of the returned region with the type of the expression. It failed when I just returned ThisVal but it passes with the fixed code.

[NoQ]

Aha, yup, thanks, this looks good!

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

146

Whitespace?

378

Let's add some reasoning, eg. "Base and delegating initializers handled above"?

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::SimpleVariableKind: {
       QualType Ty = Var->getType();
       return makeZeroElementRegion(State, State->getLValue(Var, LCtx), Ty,
                                    CallOpts.IsArrayCtorOrDtor);
     }
     case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
     case ConstructionContext::SimpleConstructorInitializerKind: {
       const auto *ICC = cast<ConstructorInitializerConstructionContext>(CC);
       const auto *Init = ICC->getCXXCtorInitializer();
-      assert(Init->isAnyMemberInitializer());
       const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
       Loc ThisPtr = SVB.getCXXThis(CurCtor, LCtx->getStackFrame());
       SVal ThisVal = State->getSVal(ThisPtr);
+      if (Init->isBaseInitializer()) {
+        const auto *ThisReg = cast<SubRegion>(ThisVal.getAsRegion());
+        const CXXRecordDecl *BaseClass =
+          Init->getBaseClass()->getAsCXXRecordDecl();
+        const auto *BaseReg =
+          MRMgr.getCXXBaseObjectRegion(BaseClass, ThisReg,
+                                       Init->isBaseVirtual());
+        return SVB.makeLoc(BaseReg);
+      }
+      if (Init->isDelegatingInitializer())
+        return ThisVal;
 
       const ValueDecl *Field;
       SVal FieldVal;
       if (Init->isIndirectMemberInitializer()) {
         Field = Init->getIndirectMember();
         FieldVal = State->getLValue(Init->getIndirectMember(), ThisVal);
       } else {
         Field = Init->getMember();
ProgramStateRef ExprEngine::updateObjectsUnderConstruction(
   case ConstructionContext::CXX17ElidedCopyVariableKind:
   case ConstructionContext::SimpleVariableKind: {
     const auto *DSCC = cast<VariableConstructionContext>(CC);
     return addObjectUnderConstruction(State, DSCC->getDeclStmt(), LCtx, V);
     }
     case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
     case ConstructionContext::SimpleConstructorInitializerKind: {
       const auto *ICC = cast<ConstructorInitializerConstructionContext>(CC);
+      const auto *Init = ICC->getCXXCtorInitializer();
+      // Base and delegating initializers handled above
+      assert(Init->isAnyMemberInitializer() &&
+             "Base and delegating initializers should have been handled by"
+             "computeObjectUnderConstruction()");
       return addObjectUnderConstruction(State, ICC->getCXXCtorInitializer(),
                                         LCtx, V);
     }
     case ConstructionContext::NewAllocatedObjectKind: {
       return State;
     }
     case ConstructionContext::SimpleReturnedValueKind:
     case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {

clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp

 namespace clang {
 namespace ento {
 namespace {
 
 class TestReturnValueUnderConstructionChecker
   : public Checker<check::PostCall> {
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
-    // Only calls with origin expression are checked. These are `returnC()`
-    // and C::C().
+    // Only calls with origin expression are checked. These are `returnC()`,
+    // `returnD()`, C::C() and D::D().
     if (!Call.getOriginExpr())
       return;
 
     // Since `returnC` returns an object by value, the invocation results
     // in an object of type `C` constructed into variable `c`. Thus the
     // return value of `CallEvent::getReturnValueUnderConstruction()` must
     // be non-empty and has to be a `MemRegion`.
     Optional<SVal> RetVal = Call.getReturnValueUnderConstruction();
     ASSERT_TRUE(RetVal);
     ASSERT_TRUE(RetVal->getAsRegion());
+
+    const auto *RetReg = cast<TypedValueRegion>(RetVal->getAsRegion());
+    const Expr *OrigExpr = Call.getOriginExpr();
+    ASSERT_EQ(OrigExpr->getType(), RetReg->getValueType());
   }
 };
 
 void addTestReturnValueUnderConstructionChecker(
     AnalysisASTConsumer &AnalysisConsumer, AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages =
     {{"test.TestReturnValueUnderConstruction", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
       Registry.addChecker<TestReturnValueUnderConstructionChecker>(
           "test.TestReturnValueUnderConstruction", "", "");
     });
 }
 
 TEST(TestReturnValueUnderConstructionChecker,
      ReturnValueUnderConstructionChecker) {
   EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
-                  R"(class C {
-                     public:
-                       C(int nn): n(nn) {}
-                       virtual ~C() {}
-                     private:
-                       int n;
-                     };
 
-                     C returnC(int m) {
-                       C c(m);
-                       return c;
-                     }
 
-                     void foo() {
-                       C c = returnC(1); 
-                     })"));
+           C c = returnC(1);
+
+  EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
+      R"(class C {
+         public:
+           C(int nn): n(nn) {}
+           explicit C(): C(0) {}
+           virtual ~C() {}
+         private:
+           int n;
+         };
+
+         C returnC() {
+           C c;
+           return c;
+         }
+
+         void foo() {
+           C c = returnC();
+         })"));
+
+  EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
+      R"(class C {
+         public:
+           C(int nn): n(nn) {}
+           virtual ~C() {}
+         private:
+           int n;
+         };
+
+         class D: public C {
+         public:
+           D(int nn): C(nn) {}
+           virtual ~D() {}
+         };
+
+         D returnD(int m) {
+           D d(m);
+           return d;
+         }
+
+         void foo() {
+           D d = returnD(1); 
+         })"));
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang