[analyzer] Skip analysis of inherited ctor as top-level function

[martong]

Summary

Skip analysis of inherited constructors as top-level functions because we
cannot model the parameters.

CXXInheritedCtorInitExpr doesn't take arguments and doesn't model
parameter initialization because there is none: the arguments in the outer
CXXConstructExpr directly initialize the parameters of the base class
constructor, and no copies are made. (Making a copy of the parameter is
incorrect, at least if it's done in an observable way.) The derived class
constructor doesn't even exist in the formal model.
E.g., in:

struct X { X *p = this; ~X() {} };
struct A { A(X x) : b(x.p == &x) {} bool b; };
struct B : A { using A::A; };
B b = X{};

... b.b is initialized to true.

[martong]

For those who are interested in more details please refer to the related discussion after the commit of the patch that introduces handling of inherited ctors.

[NoQ]

Thanks!! I also recommend a more direct test with -analyzer-display-progress | FileCheck.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522–523

That's probably the last reason why we should skip them :) I think we should focus on how these constructors don't even have a body written down in the code, so even if we find a bug, we won't be able to display it. We might as well find the bug in the inherited constructors (also /inherited/inheriting/ in your comment).

[Szelethus]

I followed the discussion, on the other patch, and this seems to be the appropriate fix -- I lack the confidence to accept, but LGTM!

[martong]

• Change comments, add FileCheck test

[martong]

In D75678#1907449, @NoQ wrote:

Thanks!! I also recommend a more direct test with -analyzer-display-progress | FileCheck.

Ok, I added a new test file with FileCheck.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522–523

Ok, I changed the comments.

[martong]

Ping.

Please prioritize this patch, since it is fixing a regression caused by https://reviews.llvm.org/D74735.

[martong]

• Remove superfluous param x from test

[NoQ]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

526

I'd rather put this Richard's comment somewhere near the respective CallEvent definition. We clearly don't need to analyze these functions, so it doesn't really matter for anybody who reads this code that there are temporary technical difficulties with analyzing them. On the other hand, it does matter a lot for people who try to understand how to implement the call event correctly.

[NoQ]

Looks great, thanks!

[martong]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

526

Ok, I moved this part of the comment into CallEvent.h .

[NoQ]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

526

Perfect, thanks!

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

public:
   static bool classof(const CallEvent *CA) {
     return CA->getKind() == CE_CXXConstructor;
   }
 };
 
 /// Represents a call to a C++ inherited constructor.
 ///
 /// Example: \c class T : public S { using S::S; }; T(1);
+///
+// Note, it is difficult to model the parameters. This is one of the reasons
+// why we skip analysis of inheriting constructors as top-level functions.
+// CXXInheritedCtorInitExpr doesn't take arguments and doesn't model parameter
+// initialization because there is none: the arguments in the outer
+// CXXConstructExpr directly initialize the parameters of the base class
+// constructor, and no copies are made. (Making a copy of the parameter is
+// incorrect, at least if it's done in an observable way.) The derived class
+// constructor doesn't even exist in the formal model.
+/// E.g., in:
+///
+/// struct X { X *p = this; ~X() {} };
+/// struct A { A(X x) : b(x.p == &x) {} bool b; };
+/// struct B : A { using A::A; };
+/// B b = X{};
+///
+/// ... b.b is initialized to true.
 class CXXInheritedConstructorCall : public AnyCXXConstructorCall {
   friend class CallEventManager;
 
 protected:
   CXXInheritedConstructorCall(const CXXInheritedCtorInitExpr *CE,
                               const MemRegion *Target, ProgramStateRef St,
                               const LocationContext *LCtx)
       : AnyCXXConstructorCall(CE, Target, St, LCtx) {}

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

 }
 
 static bool shouldSkipFunction(const Decl *D,
                                const SetOfConstDecls &Visited,
                                const SetOfConstDecls &VisitedAsTopLevel) {
   if (VisitedAsTopLevel.count(D))
     return true;
 
+  // Skip analysis of inheriting constructors as top-level functions. These
+  // constructors don't even have a body written down in the code, so even if
+  // we find a bug, we won't be able to display it.
+  if (const auto *CD = dyn_cast<CXXConstructorDecl>(D))
+    if (CD->isInheritingConstructor())
+      return true;
+
   // We want to re-analyse the functions as top level in the following cases:
   // - The 'init' methods should be reanalyzed because
   //   ObjCNonNilReturnValueChecker assumes that '[super init]' never returns
   //   'nil' and unless we analyze the 'init' functions as top level, we will
   //   not catch errors within defensive code.
   // - We want to reanalyze all ObjC methods as top level to report Retain
   //   Count naming convention errors more aggressively.
   if (isa<ObjCMethodDecl>(D))

clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp

void test_B() {
 
   // FIXME: There should be no execution path on which this is false.
   clang_analyzer_eval(b.s.y == 2); // expected-warning{{TRUE}}
                                    // expected-warning@-1{{FALSE}}
 
   clang_analyzer_eval(b.z == 3); // expected-warning{{TRUE}}
 }
 } // namespace arguments_with_constructors
+
+namespace inherited_constructor_crash {
+class a {
+public:
+  a(int);
+};
+struct b : a {
+  using a::a; // Ihnerited ctor.
+};
+void c() {
+  int d;
+  // This construct expr utilizes the inherited ctor.
+  // Note that d must be uninitialized to cause the crash.
+  (b(d)); // expected-warning{{1st function call argument is an uninitialized value}}
+}
+} // namespace inherited_constructor_crash

clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-display-progress %s 2>&1 | FileCheck %s
+
+// Test that inheriting constructors are not analyzed as top-level functions.
+
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} c()
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} a::a(int)
+// CHECK-NOT: ANALYZE (Path,  Inline_Regular): {{.*}} b::a(int)
+
+class a {
+public:
+  a(int) {}
+};
+struct b : a {
+  using a::a; // Ihnerited ctor.
+};
+void c() {
+  int d;
+  (b(d));
+  (a(d));
+}