I am not sure what to do about implcit checks. Those are probably should never be turned on or off by the user, but they should be on or off by default based on the set of checks the user enabled and the platform she is using. Thus, I am perfectly ok with the implicit_checks.html only being accessible from the checker development manual. Maybe we should extend the checker list with a notice that the user should never disable the core checks.

In D52983#1258466, @NoQ wrote:

Yay, these look useful. Is there also an attribute for methods that should never be called on a 'moved-from' object?

• Added the ideas from Kristof.

I agree that it would make sense to either not have arguments that are not represented in the AST or create expressions for those implicit arguments.

I like that we print the program points for hidden nodes :) Great work!

Oh, and thanks for working on this, this improvement was long overdue, but everybody was busy with something else :)

Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.

Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.

We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple.
But consider this as an optional task if you have nothing better to do. I am ok with committing this as is.

LGTM!

Small comments inline.

In D49693#1172605, @mikhail.ramalho wrote:

What I'm suggesting is: first run all visitors, since no other visitor mark them as invalid, the evaluation of all 497 bugs will be fast and will return only on bug report. We run the refutation manager in this one bug report.

Some comments, mostly nits inline.

Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks.

In D49074#1159095, @george.karpenkov wrote:

This may remove false-positives (like the example above), but we surely cannot find new errors where multiplicative operations are used.

Do you have examples of those? In my understanding, (almost) all of the imprecisions in the solver lead purely to false positives.
The only example where you lose bugs is when you stop exploring due to reaching a (false) fatal error.

Don't you need to edit the tests as well?

Thanks! The changes look good, I forgot to mark one double lookup though in my previous review.

Overall looks good, some nits inline. Let's run it on some projects to exercise this change.

In D48027#1142324, @MTC wrote:

• There is possible match the wrong AST node, e.g. for the NamedDecl foo(), if the function body has the a::b::x, when we match a::b::X(), we will match foo() . Because I'm not familiar with ASTMatcher, my bad, I can't think of a perfect way to match only the specified nodes.

Ok, it looks like naively just dropping all the constraints at Z3 is not the most efficient way.

LG!

LGTM!

LGTM, given that the assert does not fire for the projects you tested on.

Looks better, thanks!

Regarding the visitor:
Maybe rather than looking at the AST, we should check the states, when we started to track the returned symbol?

I wonder if this could be done when plist is emitted generally, independent of any checks.

Having C++ support would be awesome!
Thanks for working on this!

Just for the record, there is a common example where relying on copy elision might bite and google do not recommend relying on it for correctness: https://abseil.io/tips/120

In D47671#1122994, @george.karpenkov wrote:

So having an analyzer-config option would be useful for those codebases that are expected to be compiled with less advanced compilers that do not elide copies or not doing it aggressively enough.

I'm not sure it makes sense. From my understanding, that's just how c++17 is defined, and that's unrelated to what compiler implementation is used.

So having an analyzer-config option would be useful for those codebases that are expected to be compiled with less advanced compilers that do not elide copies or not doing it aggressively enough. Maybe it is worth to ask on the mailing list of the community wants to have an option for this? I am not sure though how often the end users end up fine tuning the analyzer. It might be nice to have a guide how to do that (and in what circumstances does each of the config values make sense).

Sorry for the limited activity. Unfortunately, I have very little time reviewing patches lately.
I think we need to answer the following questions:

• Does this change affect the analysis of the constructors of global objects? If so, how?
• Do we want to import non-const object's initializer expressions? The analyzer might not end up using the value anyways.
• How big can the index get with this modification for large projects?

In D45517#1116770, @george.karpenkov wrote:

I am not not sure that I got the idea what are you suggesting here. If we have the constraint of for example a symbol s > 10 and later on a path we discover s > 20, will we also deduplicate this that way?

No. But I thought in your optimization atoms inside the constraints would be the same?
Could you give an example where they are not?

In D45517#1116734, @george.karpenkov wrote:

@xazax.hun (I'll reply here to avoid scattering the conversation across many subtrees)

I was thinking about the optimization for not adding redundant constraints some more, and I've decided I'm still against it ---
we are creating a higher potential for bugs, and we are tightly coupling the visitor to an internal implementation detail (all formulas are eventually purged at purge locations),
which creates a more fragile code.

The proper way to do this would be to have a set of constraints, and then add all constraints there as we iterate through the states (and through constraints inside the state).
If we use the hashing function provided by Z3, the simple act of construction of a set would implicitly drop all redundant constraints.

LG!

LGTM!

Looks good so far, some comments inline.

The analyzer can only reason about const variables this way, right? Maybe we should only import the initializers for such variables to have better performance? What do you think?

• Rerun script

In D46234#1082307, @rsmith wrote:

Can you say something about why you want to track this?

Isn't this case already covered by conversion checker? https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

With a sufficiently detailed commit message, i.e.: what version of a project should be cheked out and how the analyzer needs to be ivoked to reproduce the problem I am ok with committing this without a test.

I am in favour of this approach. This is what I suggested back than in https://reviews.llvm.org/D23014 but it was somehow overlooked.

Overall looks good, some minor nits inline. If those can be resolved trivially, I am ok with committing this without another round of reviews.

In D45458#1062342, @NoQ wrote:

@xazax.hun: do you think the approach you took in the Valist checker is applicable here? Did you like how it ended up working? Cause i'd love to see CallDescription and initializer lists used for readability here. And i'd love to see branches replaced with map lookups. And i'm not sure if anybody has written code that has all the stuff that i'd love to see.

We encountered the same problem but did not have time yet to submit the patch. We have literally the same fix internally, so it looks good to me. One minor style nit inline.

LG! Thanks!

You should always upload the whole diff, not just the last changes.

Overall looks good, some minor comments inline.

I was thinking about test/Analysis/lambdas.cpp as a possible candidate.

I am only wondering what is the policy regarding the tests? When should we add a new file or when should we just extend an existing one?

Hmm, it looks like these changes are already upstreamed. Probably as part of other patch or maybe just someone (probably me) forgot to put the proper revision in the commit message? I close this revision now.

I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.

Note that changes from https://reviews.llvm.org/D44100 might affect this.

Hi Aleksei!

Resubmitted in https://reviews.llvm.org/rL326439

LGTM!

Could you please provide some more details where the cyclic dependency is? I cannot reproduce the problem and usually cmake fails when there is a cyclic dependency and you are building shared libraries.

Could you add some context?

In D30691#1003514, @george.karpenkov wrote:

Python code looks OK to me, I have one last request: could we have a small documentation how the whole thing is supposed work in integration, preferably on an available open-source project any reader could check out?
I am asking because I have actually tried and failed to launch this CTU patch on a project I was analyzing.

• Rebased to current ToT
• Fixed a problem that the scan-build-py used an old version of the ctu configuration option
• Added a short guide how to use CTU

@alexfh did you have any chance to think about this change?

I like the idea of having more statistics. Moreover, due to the fact that we output the percent of reachable basic blocks as an integer, it was really hard to get precise coverage data. This addition will help in this regard.

In D43131#1003607, @NoQ wrote:

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

Why not turn this on by default when we turn on reporting statistics?
Having a test for the plist output would be nice.

Thanks, this looks good to me! I will try this out soon and commit after that.

Looks good to me. Only found a few nits.