Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).

I have one small question otherwise looks good.

Thanks for the review! I will update the patch soon. As there is a dependency that is not accepted yet Richard (who authored the code I extended) might have some chance to take a look at this patch.

In D64448#1577109, @gribozavr wrote:

The tests are currently here
I think due to their dependency on a standard library, they are not a good fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and check them into clang/test.

• Address review comments.

• Fix a typo.

• Address review comments.

Some nits inline, note that this was just a partial review.

Looks good to me, some nits inline.

In D63954#1565109, @gribozavr wrote:

I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g. -wlifetime is added to the compiler invocation?

Warning flags should not change the AST -- compiler engineers don't expect that. So if Clang is going to perform inference, it should either always do it, or it should be guarded by an -f flag, not a -W flag.

LG! Thanks!

In D63954#1564448, @gribozavr wrote:

We explicitly allow to add an annotation after the definition of the class to allow adding annotations to external source from by the user

Asking users to forward-declare anything from the standard library is a very bad idea -- in fact it is undefined behavior. https://stackoverflow.com/questions/307343/forward-declare-an-stl-container

The compiler should just know about standard library types and attach attributes automatically.

LG!

I actually think we should hard code STL (and maybe some other popular) types into the compiler. I.e.: if an STL type is unannotated and the warnings are turned on, we could look up the default annotations from a table and append them during parsing. This could be done in a separate patch.

This change will be really useful for the lifetime analysis as well! Thanks!

LGTM!

Artem had some comments that are not marked as done, but LGTM!

In D62883#1552870, @Szelethus wrote:

• Now using CFGBlock::getTerminatorConditionExpr()
• Uniqueing already tracked conditions as an (Expr, ExplodedNode) pair instead of on Expr

I looked at bug path with the Optional. I did not debug into the analyzer but my intuition is the following: DefChainEnd is interesting. TerminatedPaths is the control dependency of DefChainEnd. And there are probably other things that are the control and/or value dependency of TerminatedPaths . One trick we could try is to limit the number of dependencies we track. For example, we could try what happens if we only track the immediate control dependencies of the original interesting region/symbol and does not add more control dependency recursively.

Looks good :) More diagnostics are always welcome.

Generally looks good, some nits inline. Please mark comments you already solved as done.

Oh, I realized later that code I commented on were only moved from somewhere else. If you feel like tackling these comments feel free to do so in a separate patch so this one stays clean (no changes to moved code).

I did not check the algorithm as you planned changes to that. I only did a quick review of the interface which might also be rendered obsolete once you update this patch.

Nice one :)

Once Aleksei's comments are resolved it is good to go. My comments are notes and not requests.

The only problem I see with this approach is that it is an all or nothing thing at the moment. Most of the checks in CSA can have false positives and people usually do not want to fail a build due to a false positive finding. This would force the users to do two separate passes with the static analyzer, one with the checks as errors enabled and one with the rest of the checks. The former run should not include any path sensitive checks as they are never free of false positives (due to the infeasible path problem).

Mostly looks good, I have two slightly related nits.

LG! These mistakes are so easy to make. Maybe we should add nullability annotations (or use optionals) in the future? (Or just make every non-null pointer a reference and make it a convention to always check for nulls?)

LGTM! Thanks for thefix.

Feel free to commit such trivial fixes without reviews. Alternatively, you could use LLVM_FALLTHROUGH, but I have no strong preference in this case.

Looks good, thanks. Can you commit this or do you need someone to commit it on your behalf?

I have one question, once it is resolved I am fine with committing this.

I cannot think of other users, so I would prefer to put it in the CTU lib for now.

In D46421#1456155, @r.stahl wrote:

Okay so I would suggest to go ahead and commit this. Rebased it succeeds without modification.

Still leaves the open problems with the redecls. Should I add the failing test from https://reviews.llvm.org/D46421#1375147 in the same commit marked as expected to fail? Or what is the procedure here?

In D58121#1452483, @NoQ wrote:

Mmm. I'm also pretty pinned down (it's seasonal), so i'm thinking of temporarily reverting :( until one of us gets to fixing the accidental effect on escaping, 'cause it just keeps biting us. Like, i wholeheartedly appreciate the work and i have already noticed a few times how it makes things better and loved it, it just seems to accidentally have something missing that nobody could predict, and i'll be super eager to get it back in.

First of all, sorry for the inactivity regarding this patch.

I did not check the patch yet but wanted to point out that we might not want to rush about renaming all the variables until the community decides on the coding guideline, see https://reviews.llvm.org/D59251

Did we test all the codepaths including the package level configs? If not, please add some package level config option related tests.
Otherwise the patch looks good to me after all the comments are resolved.

It is an interesting idea to use this facility for trackExpressionValue. But I would expect such a mechanism to trigger quite often. I wonder if the memory consumption would increase significantly by storing a lambda for almost every binding for each path.
Right now we reclaim the memory after we finished analyzing a top-level function. If memory proves to be a problem, we could maybe reclaim memory for every non-buggy path analyzed? Of course, I prefer the simplicity of the current solution and hope that we never need to consider more complicated cleanup logic :)

LGTM! I think we should commit this as is for now but maybe adding a TODO comment to summarize the problem would be nice. Maybe we could have an isSameDialect or similar method within LangOpts, so it is harder to break this code.

LGTM! Thanks for working on this.

The change looks good but it would be great to have a regression test as well.

In D50488#1395483, @mgrang wrote:

Reviving this now that I have some cycles to work on this.

So I tried running this on csa-testbench projects but I didn't have much success. I always run into a bunch of build/env related errors:

python run_experiments.py --config myconfig.json

15:05:20 [libcxx] Checking out project... 
[ERROR] Unknown option: json

15:05:22 [libcxx] LOC: ?.
15:05:22 [libcxx] Generating build log... 
15:05:22 [libcxx_master] Analyzing project... 
[ERROR] Traceback (most recent call last):
  File "/local/mnt/workspace/mgrang/comm_analyzer/CodeChecker/cc_bin/CodeChecker.py", line 20, in <module>
    from shared.ttypes import RequestFailed
ImportError: No module named shared.ttypes

• Fix test failures.

Experimental patch is up in https://reviews.llvm.org/D58121
Unfortunately, it is not perfect yet.

I think I might have a theory, but I would like to discuss it as I am not familiar with the internals bindings.

Just wanted to make sure I get it right. You did not add a test since it is only reproducible with an internal (non-upstreamed) checker. Since the change is trivial, I think it is ok to commit this without a test.

LG!

We have examples/analyzer-plugin. I would prefer to add an example option to the example plugin so people do see how to do this when they are registering a checker from a plugin.

LGTM! But having a lit test that fails before and passes after would be great.

In D57230#1387834, @NoQ wrote:

There seem to be a few regressions - weird memory leaks of inner objects in C++ destructors. Trying to investigate/reproduce.

Looks good, nice catch. :)

Thank you for working on this!

In D46421#1375147, @r.stahl wrote:

In D46421#1374807, @NoQ wrote:

At the same time, i don't have any test cases for the actual change in behavior that such canonicalization causes. If the test case that you had in mind is indeed demonstrating this problem, i'd love to have it. If it turns out that your test case doesn't allow us to demonstrate the problem without CTU, then probably it has something to do with ASTImporter accidentally canonicalizing the the declaration in DeclRefExpr more rarely than the vanilla AST.

This seems unrelated to CTU. The following subset of my test demonstrates this:

// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s

void clang_analyzer_eval(int);

extern const int extInt;

int main()
{
    clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
}

extern const int extInt = 2;

Breakpoint 1, (anonymous namespace)::RegionStoreManager::getBindingForVar (this=0xa7b420, B=..., R=0xa7d348)
    at /data/work/commitllvm/llvm/tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1948
1948        if (const Expr *Init = VD->getAnyInitializer()) {
(gdb) p VD->getInit()
$1 = (const clang::Expr *) 0x0
(gdb) p VD->getAnyInitializer()
$2 = (const clang::Expr *) 0xa4b630

Thanks for all the reviews. Do you have any preference about the spelling of the annotation mentioned in the description?

I tried to creduce one file where the result differed and this is the result:

typedef struct {
  int a;
  int b
} c;
d;
e(c *f) {
  d < f->a;
  c g;
  h(&g.b);
  e(&g);
}

In D57230#1372523, @NoQ wrote:

In D57230#1372488, @xazax.hun wrote:

In D57230#1372275, @NoQ wrote:

Do you have success reducing false positives using creduce? My problem usually is that we cannot tell if a reduction rendered a false positive into a true positive.

False positives - no. Improvements and regressions - totally! Just run two different clangs in the creduce test and check that there's a difference in results.

• Added some tests

In D57230#1372275, @NoQ wrote:

Could you share reproducible examples for these, probably in the form of FIXME tests? Given that they are "regressions", they are easy to creduce down to a small repro by using the test "there is still a change in behavior on this file".

To add an analogy, Clang Tidy will not require C++ Core Guidelines related checks to be evaluated on projects that are not following the guidelines as the results are meaningless for those projects.

In D35068#1361902, @Szelethus wrote:

In D35068#1069880, @koldaniel wrote:

I've evaluated this checker on LLVM+Clang, there were only a few (about 15) warnings, because of the C11 flag check at the beginning of the checker body. However, if this check was removed, number of the warnings would be increased significantly. I wouldn't say the findings were real security issues, most of the warnings were about usages of deprecated functions, which has not been considered unsecure (but which may cause problems if the code is modified in an improper way in the future).

My problem is that LLVM+Clang isn't really a C (nor a C11) project, and I think judging this checker on it is a little misleading. Could you please test it on some C11 projects? I think tmux uses C11.

Edit: it doesn't, but CMake is mostly a C project and it does!

Any objections to commit this?
I think this is quiet coding guideline specific check which is useful for a set of security critical projects. As this is an opt in kind of check, I think it does no harm to have it upstream.

Thanks, LGTM! It is interesting to see if we need to traverse all the super regions in scanReachableSymbols, but if we need to change something there, I would prefer that to be in a separate patch.
If visiting the whole super region chain proved to be redundant I would recommend removing it for clarity regardless of having a performance impact.

I really like all this detective work and it would be sad to have it forgotten. I would love to see some of your comments in the documentation of symbol reaper.
More specifically:

Some nits inline. Otherwise looks good to me.

Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.

Sorry for the delay. The changes are looking good to me, although I think it might be worth to split this up into two patch, one NFC with the renaming, and one that actually introduces the changes.

LG!

LG!

While Static Analyzer is the only client of CTU library at the moment, we might have more in the future. I would not use the phrase ANALYZE in the log message. Once this is resolved the rest looks good.

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

LG!