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.

In D5767#999143, @sabel83 wrote:

2. What do you mean by regression tests? We have run the clang-test target successfully on the patched code (which has the hook). Note that the hook this pull request provides is implemented as a ProgramAction. It is not intended to be used together with other program actions, I don't see how we could turn it on for other tests (if that is what you referred to).

Overall looks good. Was this tested on large software? I would also be grateful if you could run the regression tests with templight always being enabled to see if they uncover any assertions/crashes.

• Address review comments.

Overall looks good! Thanks for working on this!

In D42301#986583, @a.sidorin wrote:

I'd rather create a separate patch - this one is already large enough. Is it OK?

• Added test for CXXTypeIdExpr import.

• Added import for CXXTypeidExpr. What is the best way to test this? <typeinfo> header is required for using the typeid operator, but relying on the presence of an STL library in tests is usually considered as a bad practice. Should we mock a typeinfo implementation just for this purpose?

In D42301#984713, @a.sidorin wrote:

I do not see a test for the following changes:

• ASTImporter: don't add templated declarations into DeclContext

It's in ASTImporterTest. It checks that the templated decl cannot be found in the enclosing TU.

I do not see a test for the following changes:

• ASTImporter: don't add templated declarations into DeclContext
• ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc

High level note: clang::TypeAliasDecl has the same issue as CXXRecordDecl, so templated versions should not be added to the DeclContext. Could you add that just for the sake of completeness?

In D42335#983878, @a.sidorin wrote:

Hello Peter,

Thank you for the patch! It is almost LGTM, just a few minor questions inline.
Am I understand correctly that it is partially based on https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp?
Also, FYI: you can use ASTMerge and clang-import-test facilities for testing. ASTMatchers are completely fine, but usage of imported AST samples as Sema lookup source can uncover some subtle issues in the built AST.

• Address review comments.

This should be rebased to latest master.

• Fixed review comments

I like this patch, only found a few nits after the other's review.

I am fine with suppressing the escape globally.
I did see some code in the wild where the constructors registered the objects with a (global) map.
But I think it is still easier to annotate code that does something unconventional than the other way around.

(@xazax.hun - this is an alpha checker last touched by you, do you still have plans for it?)

Do you have a plan for the new false negatives when c++-allocator-inlining is on? Should the user mark allocation functions with attributes?

LG!

Great! Thanks!

Overall looks good to me, one comment inline. I think it is good to have these checks to prevent the analyzer executing undefined behavior. Maybe this would make it more feasible to run the analyzer with ubsan :)
In the future, it would be great to also look for these cases symbolically, but I believe it is perfectly fine to have that in a separate patch.

A high level comment: while it is very easy to get all the gotos using grep, it is not so easy to get all the labels. So for this check to be complete, I think it would be useful to also find labels (possibly having a configuration option for that).

In D41538#969205, @sylvestre.ledru wrote:

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

Hi Hans!

https://reviews.llvm.org/D41538 is superior and committed.

• Address some review comments.

• Address review comments.