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

[Artem Dergachev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5e0fb6484207: [analyzer] Add test cases for the unsupported 
C++ constructor modeling. (authored by dergachev.a).

Changed prior to commit:
  https://reviews.llvm.org/D69308?vs=227714&id=228344#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69308

Files:
  clang/test/Analysis/handle_constructors_for_default_arguments.cpp
  clang/test/Analysis/handle_constructors_with_new_array.cpp
  clang/www/analyzer/open_projects.html

Index: clang/www/analyzer/open_projects.html
===
--- clang/www/analyzer/open_projects.html
+++ clang/www/analyzer/open_projects.html
@@ -68,7 +68,7 @@
 
   Improve C++ support
   
-Handle aggregate construction.
+Handle construction as part of aggregate initialization.
   https://en.cppreference.com/w/cpp/language/aggregate_initialization";>Aggregates
   are objects that can be brace-initialized without calling a
   constructor (that is, https://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html";>
@@ -89,12 +89,31 @@
   (Difficulty: Medium) 
 
 
-Handle constructors within new[]
-  When an array of objects is allocated using the operator new[],
+Handle array constructors.
+  When an array of objects is allocated (say, using the
+ operator new[] 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
  operator delete[].
+ See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/handle_constructors_with_new_array.cpp";>handle_constructors_with_new_array.cpp.
+  
+  
+  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.
   
 
 
@@ -116,6 +135,24 @@
 Handle constructors for default arguments
   Default arguments in C++ are recomputed at every call,
  and are therefore local, and not static, variables.
+ See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/handle_constructors_for_default_arguments.cpp";>handle_constructors_for_default_arguments.cpp.
+  
+  
+  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).
   
 
 
Index: clang/test/Analysis/handle_constructors_with_new_array.cpp
===
--- /dev/null
+++ clang/test/Analysis/handle_constructors_with_new_array.cpp
@@ -0,0 +1,86 @@
+// 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.
+
+/

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Thanks!


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

https://reviews.llvm.org/D69308



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

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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

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


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

https://reviews.llvm.org/D69308



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

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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

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.


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

https://reviews.llvm.org/D69308



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

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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp updated this revision to Diff 227714.
dkrupp marked 2 inline comments as done.
dkrupp added a comment.

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


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

https://reviews.llvm.org/D69308

Files:
  
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp
  
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
  clang/www/analyzer/open_projects.html

Index: clang/www/analyzer/open_projects.html
===
--- clang/www/analyzer/open_projects.html
+++ clang/www/analyzer/open_projects.html
@@ -95,6 +95,26 @@
  We should model (potentially some of) such evaluations,
  and the same applies for destructors called from
  operator delete[].
+ See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp";>handle_constructors_with_new_array.cpp.
+  
+  
+  Hints for implementation by Artem Dergachev (NoQ):
+  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.
+  
   
 
 
@@ -117,6 +137,25 @@
   Default arguments in C++ are recomputed at every call,
  and are therefore local, and not static, variables.
   
+  See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp";>handle_constructors_for_default_arguments.cpp.
+  
+  Hints for implementation by Artem Dergachev (NoQ):
+  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).
+  
 
 
 Enhance the modeling of the standard library.
Index: clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
===
--- /dev/null
+++ clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
@@ -0,0 +1,86 @@
+// 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_automati

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69308



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.




Comment at: 
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp:3-6
+// REQUIRES: non-existing-system
+
+// These test cases demonstrate lack of Static Analyzer features.
+// Remove REQUIRES line, when the feature gets implemented.

NoQ wrote:
> I prefer our FIXME-tests to keep running while documenting incorrect results, 
> so that people were informed when they fix something. Eg.:
> 
> ```lang=c++
> // 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.
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69308



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

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

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

Szelethus added a comment.

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 :^)


Repository:
  rC Clang

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

https://reviews.llvm.org/D69308



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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




Comment at: 
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp:3-6
+// REQUIRES: non-existing-system
+
+// These test cases demonstrate lack of Static Analyzer features.
+// Remove REQUIRES line, when the feature gets implemented.

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

```lang=c++
// 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.



Comment at: 
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp:58
+
+int called_h(init_default_member l = init_default_member()) {
+  //We expect that the analyzer assumes the default value (call site test3)

Note that default arguments may also be of reference type, eg.:
```lang=c++
void foo(const X &x = X(1, 2, 3));
void bar(Y &&y = Y(4, 5, 6));
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D69308



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

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

[Daniel Krupp via Phabricator via cfe-commits]

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

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


Repository:
  rC Clang

https://reviews.llvm.org/D69308

Files:
  
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp
  
clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
  clang/www/analyzer/open_projects.html

Index: clang/www/analyzer/open_projects.html
===
--- clang/www/analyzer/open_projects.html
+++ clang/www/analyzer/open_projects.html
@@ -95,6 +95,7 @@
  We should model (potentially some of) such evaluations,
  and the same applies for destructors called from
  operator delete[].
+ See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp";>handle_constructors_with_new_array.cpp.
   
 
 
@@ -117,6 +118,7 @@
   Default arguments in C++ are recomputed at every call,
  and are therefore local, and not static, variables.
   
+  See tests cases in https://github.com/llvm/llvm-project/tree/master/clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp";>handle_constructors_for_default_arguments.cpp.
 
 
 Enhance the modeling of the standard library.
Index: clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
===
--- /dev/null
+++ clang/test/Analysis/clangsa_unsupported_features/handle_constructors_with_new_array.cpp
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection %s -verify
+// REQUIRES: non-existing-system
+
+// These test cases demonstrate lack of Static Analyzer features.
+// Remove REQUIRES line, when the feature gets implemented.
+
+// 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];
+
+  clang_analyzer_eval(a1[0].a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a2[0].a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a3[0].a == 1); // expected-warning {{TRUE}}
+}
+
+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];
+
+  clang_analyzer_eval(a1[0].a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a2[0].a == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(a3[0].a == 1); // expected-warning {{TRUE}}
+
+  delete[] a1;
+  delete[] a2;
+  delete[] a3;
+}
Index: clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp
===
--- /dev/null
+++ clang/test/Analysis/clangsa_unsupported_features/handle_constructors_for_default_arguments.cpp
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection %s -verify
+// REQUIRES: non-existing-system
+
+// These test cases demonstrate lack of Static Analyzer features.
+// Remove REQUIRES line, when the feature gets implemented.
+
+// Handle constructors for default arguments
+// Default arguments