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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:511
+  //
+  // Also, we cannot model the parameters. CXXInheritedCtorInitExpr doesn't
+  // take arguments and doesn't model parameter initialization because there is

martong wrote:
> NoQ wrote:
> > 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.
> Ok, I moved this part of the comment into CallEvent.h .
Perfect, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rG59a960b83c2d: [analyzer] Skip analysis of inherited ctor as 
top-level function (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D75678?vs=248678=249058#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
  clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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));
+}
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -519,6 +519,13 @@
   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(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
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -896,6 +896,23 @@
 /// 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 == ) {} bool b; };
+/// struct B : A { using A::A; };
+/// B b = X{};
+///
+/// ... b.b is initialized to true.
 class CXXInheritedConstructorCall : public AnyCXXConstructorCall {
   friend class CallEventManager;
 


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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,  

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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:511
+  //
+  // Also, we cannot model the parameters. CXXInheritedCtorInitExpr doesn't
+  // take arguments and doesn't model parameter initialization because there is

NoQ wrote:
> 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.
Ok, I moved this part of the comment into CallEvent.h .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:511
+  //
+  // Also, we cannot model the parameters. CXXInheritedCtorInitExpr doesn't
+  // take arguments and doesn't model parameter initialization because there is

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 248678.
martong added a comment.

- Remove superfluous param x from test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678

Files:
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
  clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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));
+}
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -504,6 +504,29 @@
   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.
+  //
+  // Also, 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 == ) {} bool b; };
+  // struct B : A { using A::A; };
+  // B b = X{};
+  //
+  // ... b.b is initialized to true.
+  if (const auto *CD = dyn_cast(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


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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));
+}
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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}}
+}

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 248473.
martong marked an inline comment as done.
martong added a comment.

- Change comments, add FileCheck test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678

Files:
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
  clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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(int)
+// 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 x) {
+  int d;
+  (b(d));
+  (a(x));
+}
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -504,6 +504,29 @@
   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.
+  //
+  // Also, 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 == ) {} bool b; };
+  // struct B : A { using A::A; };
+  // B b = X{};
+  //
+  // ... b.b is initialized to true.
+  if (const auto *CD = dyn_cast(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


Index: clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-is-skipped-as-top-level.cpp
@@ -0,0 +1,20 @@
+// 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(int)
+// 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 x) {
+  int d;
+  (b(d));
+  (a(x));
+}
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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)); // 

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

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

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.




Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:507-508
 
+  // Skip analysis of inherited constructors as top-level functions because we
+  // cannot model the parameters.
+  //

NoQ wrote:
> 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).
Ok, I changed the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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




Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:507-508
 
+  // Skip analysis of inherited constructors as top-level functions because we
+  // cannot model the parameters.
+  //

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added a reviewer: NoQ.
Herald added subscribers: cfe-commits, steakhal, Charusso, gamesh411, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, xazax.hun, whisperity.
Herald added a reviewer: Szelethus.
Herald added a project: clang.

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 == ) {} bool b; };
struct B : A { using A::A; };
B b = X{};

... b.b is initialized to true.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75678

Files:
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp


Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -504,6 +504,27 @@
   if (VisitedAsTopLevel.count(D))
 return true;
 
+  // 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 == ) {} bool b; };
+  // struct B : A { using A::A; };
+  // B b = X{};
+  //
+  // ... b.b is initialized to true.
+  if (const auto *CD = dyn_cast(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


Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -57,3 +57,19 @@
   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
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -504,6 +504,27 @@
   if (VisitedAsTopLevel.count(D))
 return true;
 
+  // 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
+  //