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.

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.