[analyzer] Test cases for the unsupported features for Clang Static Analyzer

[dkrupp]

Summary

These test cases demonstrate some of the missing features of the Clang Static Analyzer.

In this patch 2 missing C++ features are demonstrated from https://clang-analyzer.llvm.org/open_projects.html

1. Handle constructors within new[]
2. Handle constructors for default arguments

[NoQ]

Thanks for the tests!

Both of these features are relatively hard.

Operator new[] requires invoking multiple (potentially unknown) amount of constructors with the same construct-expression. Apart from the technical difficulties of juggling program points around correctly to avoid accidentally merging paths together, you'll have to be a judge on when to exit the loop and how to widen it. Given that the constructor is going to be a default constructor, a nice 95% solution might be to execute exactly one constructor and then default-bind the resulting LazyCompoundVal to the whole array; it'll work whenever the default constructor doesn't touch global state but only initializes the object to various default values. But if, say, you're making an array of strings, depending on the implementation you might have to allocate a new buffer for each string, and in this case default-binding won't cut it. You might want to come up with an auxiliary analysis in order to perform widening of these simple loops more precisely.

Default arguments are annoying because the initializer expression is evaluated at the call site but doesn't syntactically belong to the caller's AST; instead it belongs to the ParmVarDecl for the default parameter. This can lead to situations when the same expression has to carry different values simultaneously - when multiple instances of the same function are evaluated as part of the same full-expression without specifying the default arguments. Even simply calling the function twice (not necessarily within the same full-expression) may lead to program points agglutinating because it's the same expression. There are some nasty test cases already in temporaries.cpp (struct DefaultParam and so on). I recommend adding a new LocationContext kind specifically to deal with this problem. It'll also help you figure out the construction context when you evaluate the construct-expression (though you might still need to do some additional CFG work to get construction contexts right).

clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp

3–6 ↗

(On Diff #226053)

I prefer our FIXME-tests to keep running while documenting incorrect results, so that people were informed when they fix something. Eg.:

// FIXME: Should be TRUE.
clang_analyzer_eval(x); // expected-warning{{UNKNOWN}}

I doubt that the whole file of tests will be fixed by a single commit, so they'll have to change this anyway.

It's also nice to inform people that they accidentally changed something they didn't expect to change.

58 ↗

(On Diff #226053)

Note that default arguments may also be of reference type, eg.:

void foo(const X &x = X(1, 2, 3));
void bar(Y &&y = Y(4, 5, 6));

[Szelethus]

LGTM given that the inlines are fixed.

In D69308#1722139, @NoQ wrote:

Thanks for the tests!

Both of these features are relatively hard.

...

Would love to see this comment in its entirety on the open projects page :^)

[NoQ]

In D69308#1727108, @Szelethus wrote:

Would love to see this comment in its entirety on the open projects page :^)

I'd rather have a mention that @dkrupp is already working on this project, so that if somebody wanted to help out they could cooperate nicely.

clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp

3–6 ↗

(On Diff #226053)

Also let's not put it to "unsupported_features" so that we didn't have to move it (and lose history / or forget to move) later.

[NoQ]

Another interesting problem that we forgot to mention on the open projects page is the modeling of C++17 bindings and decompositions: https://bugs.llvm.org/show_bug.cgi?id=43042

Also, in my opinion, out of all construction context problems mentioned on the open projects page, the NRVO problem is probably the easiest. It might as well be the least rewarding of them, but i think it is the friendliest possible problem to start with, as it doesn't force you to invent large new facilities.

[dkrupp]

Thanks for your comments @NoQ
I fixed them. Also added your implementation hints to the open projects page.

[dkrupp]

In D69308#1727587, @NoQ wrote:

In D69308#1727108, @Szelethus wrote:

Would love to see this comment in its entirety on the open projects page :^)

I'd rather have a mention that @dkrupp is already working on this project, so that if somebody wanted to help out they could cooperate nicely.

I am not working on this yet. First I would like to explore with these test cases what exactly is not working as expected.

[dkrupp]

In D69308#1727625, @NoQ wrote:

Another interesting problem that we forgot to mention on the open projects page is the modeling of C++17 bindings and decompositions: https://bugs.llvm.org/show_bug.cgi?id=43042

Also, in my opinion, out of all construction context problems mentioned on the open projects page, the NRVO problem is probably the easiest. It might as well be the least rewarding of them, but i think it is the friendliest possible problem to start with, as it doesn't force you to invent large new facilities.

Thanks. I will try to create a simple reproducer for this too and add this to the open projects page (in another patch).

[NoQ]

Thanks!

clang/test/Analysis/handle_constructors_for_default_arguments.cpp

+// RUN: %clang_cc1 -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection %s -verify
+
+// These test cases demonstrate lack of Static Analyzer features.
+// The FIXME: tags indicate where we expect different output.
+
+// Handle constructors for default arguments.
+// Default arguments in C++ are recomputed at every call,
+// and are therefore local, and not static, variables.
+void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
+
+struct init_with_list {
+  int a;
+  init_with_list() : a(1) {}
+};
+
+struct init_in_body {
+  int a;
+  init_in_body() { a = 1; }
+};
+
+struct init_default_member {
+  int a = 1;
+};
+
+struct basic_struct {
+  int a;
+};
+
+// Top-level analyzed functions.
+void top_f(init_with_list l = init_with_list()) {
+  // We expect that the analyzer doesn't assume anything about the parameter.
+  clang_analyzer_eval(l.a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void top_g(init_in_body l = init_in_body()) {
+  // We expect that the analyzer doesn't assume anything about the parameter.
+  clang_analyzer_eval(l.a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void top_h(init_default_member l = init_default_member()) {
+  // We expect that the analyzer doesn't assume anything about the parameter.
+  clang_analyzer_eval(l.a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+// Not-top-level analyzed functions.
+int called_f(init_with_list l = init_with_list()) {
+  // We expect that the analyzer assumes the default value
+  // when called from test2().
+  return l.a;
+}
+
+int called_g(init_in_body l = init_in_body()) {
+  // We expect that the analyzer assumes the default value
+  // when called from test3().
+  return l.a;
+}
+
+int called_h(init_default_member l = init_default_member()) {
+  // We expect that the analyzer assumes the default value
+  // when called from test4().
+  return l.a;
+}
+
+int called_i(const init_with_list &l = init_with_list()){
+  // We expect that the analyzer assumes the default value
+  // when called from test5().
+  return l.a;
+}
+
+int called_j(init_with_list &&l = init_with_list()){
+  // We expect that the analyzer assumes the default value
+  // when called from test6().
+  return l.a;
+}
+
+int plain_parameter_passing(basic_struct l) {
+  return l.a;
+}
+
+void test1() {
+  basic_struct b;
+  b.a = 1;
+  clang_analyzer_eval(plain_parameter_passing(b) == 1); //expected-warning {{TRUE}}
+}
+
+void test2() {
+  // We expect that the analyzer assumes the default value.
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(called_f() == 1); //expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void test3() {
+  // We expect that the analyzer assumes the default value.
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(called_g() == 1); //expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void test4() {
+  // We expect that the analyzer assumes the default value.
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(called_h() == 1); //expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void test5() {
+  //We expect that the analyzer assumes the default value.
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(called_i() == 1); //expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void test6() {
+  // We expect that the analyzer assumes the default value.
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(called_j() == 1); //expected-warning {{TRUE}} expected-warning {{FALSE}}
+}

clang/test/Analysis/handle_constructors_with_new_array.cpp

+// RUN: %clang_cc1 -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection %s -verify
+
+// These test cases demonstrate lack of Static Analyzer features.
+// The FIXME: tags indicate where we expect different output.
+
+// Handle constructors within new[].
+
+// When an array of objects is allocated using the operator new[],
+// constructors for all elements of the array are called.
+// We should model (potentially some of) such evaluations,
+// and the same applies for destructors called from operator delete[].
+
+void clang_analyzer_eval(bool);
+
+struct init_with_list {
+  int a;
+  init_with_list() : a(1) {}
+};
+
+struct init_in_body {
+  int a;
+  init_in_body() { a = 1; }
+};
+
+struct init_default_member {
+  int a = 1;
+};
+
+void test_automatic() {
+
+  init_with_list a1;
+  init_in_body a2;
+  init_default_member a3;
+
+  clang_analyzer_eval(a1.a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a2.a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a3.a == 1); // expected-warning {{TRUE}}
+}
+
+void test_dynamic() {
+
+  auto *a1 = new init_with_list;
+  auto *a2 = new init_in_body;
+  auto *a3 = new init_default_member;
+
+  clang_analyzer_eval(a1->a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a2->a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a3->a == 1); // expected-warning {{TRUE}}
+
+  delete a1;
+  delete a2;
+  delete a3;
+}
+
+void test_automatic_aggregate() {
+
+  init_with_list a1[1];
+  init_in_body a2[1];
+  init_default_member a3[1];
+
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a1[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a2[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a3[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+}
+
+void test_dynamic_aggregate() {
+
+  auto *a1 = new init_with_list[1];
+  auto *a2 = new init_in_body[1];
+  auto *a3 = new init_default_member[1];
+
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a1[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a2[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+  // FIXME: Should be TRUE, not FALSE.
+  clang_analyzer_eval(a3[0].a == 1); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+
+  delete[] a1;
+  delete[] a2;
+  delete[] a3;
+}

clang/www/analyzer/open_projects.html

<ul>
         may produce much better results.</p>
         <p><i>(Difficulty: Medium)</i></p>
       </li>
     </ul>
   </li>
 
   <li>Improve C++ support
   <ul>
-    <li>Handle aggregate construction.
+    <li>Handle construction as part of aggregate initialization.
       <p><a href="https://en.cppreference.com/w/cpp/language/aggregate_initialization">Aggregates</a>
       are objects that can be brace-initialized without calling a
       constructor (that is, <code><a href="https://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html">
       CXXConstructExpr</a></code> does not occur in the AST),
       but potentially calling
       constructors for their fields and base classes
       These
       constructors of sub-objects need to know what object they are constructing.
       Moreover, if the aggregate contains
       references, lifetime extension needs to be properly modeled.
 
       One can start untangling this problem by trying to replace the
       current ad-hoc <code><a href="https://clang.llvm.org/doxygen/classclang_1_1ParentMap.html">
       ParentMap</a></code> lookup in <a href="https://clang.llvm.org/doxygen/ExprEngineCXX_8cpp_source.html#l00430">
       <code>CXXConstructExpr::CK_NonVirtualBase</code></a> branch of
       <code>ExprEngine::VisitCXXConstructExpr()</code>
       with proper support for the feature.
       <p><i>(Difficulty: Medium) </i></p></p>
     </li>
 
-    <li>Handle constructors within <code>new[]</code>
-      <p>When an array of objects is allocated using the <code>operator new[]</code>,
+    <li>Handle array constructors.
+      <p>When an array of objects is allocated (say, using the
+         <code>operator new[]</code> or defining a stack array),
          constructors for all elements of the array are called.
          We should model (potentially some of) such evaluations,
          and the same applies for destructors called from
          <code>operator delete[]</code>.
+         See tests cases in <a href="https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/handle_constructors_with_new_array.cpp">handle_constructors_with_new_array.cpp</a>.
+      </p>
+      <p>
+      Constructing an array requires invoking multiple (potentially unknown)
+      amount of constructors with the same construct-expression.
+      Apart from the technical difficulties of juggling program points around
+      correctly to avoid accidentally merging paths together, we'll have to
+      be a judge on when to exit the loop and how to widen it.
+      Given that the constructor is going to be a default constructor,
+      a nice 95% solution might be to execute exactly one constructor and
+      then default-bind the resulting LazyCompoundVal to the whole array;
+      it'll work whenever the default constructor doesn't touch global state
+      but only initializes the object to various default values.
+      But if, say, we're making an array of strings,
+      depending on the implementation you might have to allocate a new buffer
+      for each string, and in this case default-binding won't cut it.
+      We might want to come up with an auxiliary analysis in order to perform
+      widening of these simple loops more precisely.
       </p>
     </li>
 
     <li>Handle constructors that can be elided due to Named Return Value Optimization (NRVO)
       <p>Local variables which are returned by values on all return statements
          may be stored directly at the address for the return value,
          eliding the copy or move constructor call.
          Such variables can be identified using the AST call <code>VarDecl::isNRVOVariable</code>.
       </p>
     </li>
 
     <li>Handle constructors of lambda captures
       <p>Variables which are captured by value into a lambda require a call to
          a copy constructor.
          This call is not currently modeled.
       </p>
     </li>
 
     <li>Handle constructors for default arguments
       <p>Default arguments in C++ are recomputed at every call,
          and are therefore local, and not static, variables.
+         See tests cases in <a href="https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/handle_constructors_for_default_arguments.cpp">handle_constructors_for_default_arguments.cpp</a>.
+      </p>
+      <p>
+      Default arguments are annoying because the initializer expression is
+      evaluated at the call site but doesn't syntactically belong to the
+      caller's AST; instead it belongs to the ParmVarDecl for the default
+      parameter. This can lead to situations when the same expression has to
+      carry different values simultaneously -
+      when multiple instances of the same function are evaluated as part of the
+      same full-expression without specifying the default arguments.
+      Even simply calling the function twice (not necessarily within the
+      same full-expression) may lead to program points agglutinating because
+      it's the same expression. There are some nasty test cases already
+      in temporaries.cpp (struct DefaultParam and so on). I recommend adding a
+      new LocationContext kind specifically to deal with this problem. It'll
+      also help you figure out the construction context when you evaluate the
+      construct-expression (though you might still need to do some additional
+      CFG work to get construction contexts right).
       </p>
     </li>
 
     <li>Enhance the modeling of the standard library.
       <p>The analyzer needs a better understanding of STL in order to be more
       useful on C++ codebases.
       While full library modeling is not an easy task,
       large gains can be achieved by supporting only a few cases: