Page MenuHomePhabricator

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

Authored by martong on Mar 5 2020, 5:26 AM.

Details

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.

Diff Detail

Event Timeline

martong created this revision.Mar 5 2020, 5:26 AM
Herald added a project: Restricted Project. · View Herald Transcript

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 added a comment.Mar 5 2020, 5:42 AM

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

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
507–508

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

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 updated this revision to Diff 248473.Mar 5 2020, 6:50 AM
martong marked an inline comment as done.
  • Change comments, add FileCheck test
martong marked an inline comment as done.Mar 5 2020, 6:52 AM
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
507–508

Ok, I changed the comments.

Ping.

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

martong updated this revision to Diff 248678.Mar 6 2020, 2:36 AM
  • Remove superfluous param x from test
NoQ added inline comments.Mar 7 2020, 8:53 AM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
511

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 accepted this revision.Mar 7 2020, 8:53 AM

Looks great, thanks!

This revision is now accepted and ready to land.Mar 7 2020, 8:53 AM
martong marked 2 inline comments as done.Mar 9 2020, 4:04 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
511

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

This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
NoQ added inline comments.Mar 15 2020, 7:16 PM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
511

Perfect, thanks!