Page MenuHomePhabricator

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

Authored by dkrupp on Oct 22 2019, 8:47 AM.

Details

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

Diff Detail

Event Timeline

dkrupp created this revision.Oct 22 2019, 8:47 AM
NoQ added a comment.Oct 25 2019, 7:48 PM

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 added a comment.EditedOct 30 2019, 8:02 AM

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

Szelethus accepted this revision.Oct 30 2019, 8:02 AM
This revision is now accepted and ready to land.Oct 30 2019, 8:02 AM
NoQ added a comment.Oct 30 2019, 12:08 PM

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 added a comment.Oct 30 2019, 12:23 PM

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 updated this revision to Diff 227714.Nov 4 2019, 8:25 AM
dkrupp marked 2 inline comments as done.

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

dkrupp added a comment.Nov 4 2019, 8:48 AM
In D69308#1727587, @NoQ 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 added a comment.Nov 4 2019, 8:50 AM
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 accepted this revision.Nov 4 2019, 10:11 AM

Thanks!

This revision was automatically updated to reflect the committed changes.