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!

Having an analyzer config option makes sense.

The code LGTM! I am not good at wordsmithing, but if the descriptions of the statistics are not clear enough, I agree that they should be rephrased.

After the review comment is resolved, the rest LGTM!

• Addressed further comments.

Overall looks good to me, some minor comments inline.

Some minor comment inline. Otherwise looks good.

In D54557#1300654, @NoQ wrote:

In D54557#1299736, @xazax.hun wrote:

It would be great to have a way to extend the list of (possibly non-stl) types to check. But I do understand that the analyzer does not have a great way to set such configuration options right now.

Do you envision room for another attribute here? I.e., a class attribute that says "this object is always unsafe to use after move, unless a method annotated with reinitializes is called"?

It would be great to have a way to extend the list of (possibly non-stl) types to check. But I do understand that the analyzer does not have a great way to set such configuration options right now.

Looks good. Do we plan to detect problems other than use after move? Maybe it would be worth to synchronize with the tidy checker name use-after-move or is it going to cause more confusion?

I do like the idea of moving the Clang Static Analyzer documentation to where the rest of the tools are documented. I believe the original reason the analyzer had a separate homepage is due to it was off by default in clang at the beginning and users downloaded it from the separate page.

• Use the term checker instead of check.

• Move the checklist up before additional info in the HTML file.
• Fix minor nits.
• Add missing bullet points (thanks @Szelethus for noticing)

I would love to see a test with deeper macro in macro expansion and larger number of arguments, with some of the arguments unused. Some minor nits inline, otherwise looks good.

This new version based on the bullets by NoQ. I also included some additional ones from other lists and added some new ones, e.g. the NamedDecl::getName will fail if the name of the decl is not a single token. I also reordered a bit. Advice that is more advanced and guidelines that are less likely to be violated should be closer to the bottom of the list.

LGTM, but let's wait for @NoQ before committing.

I also would like to see in a tool how this would look like as an event before committing :) Just a sanity check to make sure this feature makes sense. Could you post a screenshot of CodeChecker or any other tool using this feature?

One question otherwise looks good.

One question and one nit otherwise looks good. Feel free to commit once those are resolved without another round of reviews.

• Remove yet another dependency from the tool that is no longer used.

Please add a test case where a bug path goes through a macro definition and this macro is undefed at the end of the translation unit.

LGTM! Thanks, I think it is much easier to understand what is going on this way.

LGTM!
I only wonder if this should be on by default or guarded by a config option. I do not have strong feelings about any of the options though.

I agree with NoQ. Forward and backward compatibility might be important for CodeChecker as well.
But I wonder if it make sense to have analyzer-config compatibility mode on a per config basis?
E.g., if we have two configs:

• One did not exist in earlier clang versions, but a tool (like CodeChecker) would like to pass this to the analyzer without doing a version check first. Passing this in a compatibility mode makes sense. This could be the regural -analyzer-config
• The second option also did not exist in earlier clang versions, but we do not want to support those versions. In the case passing this config in a more strict mode makes sense. This could be something like -analyzer-config-strict.

Overall looks good, minor comments inline.

Overall looks good if the community agrees with the directions. Some comments inline.

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.