Committed in 343bdb10940cb2387c0b9bd3caccee7bb56c937b.

@steakhal thanks for the review. I fixed all outstanding remarks.
I left the test taint-diagnostic-visitor.c formatting as is to remain consistent with the rest of the file. I think we should keep it as is, or reformat the whole file.

-using llvm::ArrayRef<SymbolRef> in the reportTaintBug(..) function in the DivZero Checker

-append_range(..) used instead of std::vector.insert(...) to improve readability
-minor updates based on @steakhal comments

@steakhal your comments are fixed. Thanks for the review.

-getTaintedSymbols(.) -> getTaintedSymbolsImpl() proxy function introduced for interface safety
-Other minor fixes based on comments from @steakhal

@steakhal is there anything else to do before we merge this? Thanks.

@steakhal thanks for your review. All your remarks have been fixed.

• Implemented early return in getTaintedSymbols() when it is called by isTainted() for efficiency
• Fixed test incompatibility on Windows

You can find the improved reports on tmux, postgres, twin, openssl here:

All remarks from @steakhal has been fixed. Thanks for the review.
This new version now can handle the tracking back of multiple symbols!

-All remarks from @steakhal was fixed. Thanks for the review!
-Now we can generate diagnostics for all tainted values when they reach a sink.

All comments addressed. Thanks for the review @steakhal .

@steakhal thanks for your review. I tried to address all your concerns.
I added an extra test case too (multipleTaintSources(..)) which highlights the limitation of the current patch: If multiple tainted "variables" reach a sink, we only generate diagnostics for one of them. The main reason is that the isTainted() function returns a single tainted Symbolref instead of a vector of Symbolrefs if there are multiple instances.
I highlighted this in the test and the implementation.

This is a totally rewritten version of the patch which solely relies on the existing "interestingness" utility to track back the taint propagation. (And does not introduce a new FlowID in the ProgramState as requested in the reviews.)

@steakhal , @NoQ thanks for the reviews. I will try to implement an alternative solution based on your suggestions.

TaintBugReport is brilliant and we already have a precedent for subclassing BugReport in another checker. However I'm somewhat worried that once we start doing more of this, we'll eventually end up with multiple inheritance situations when the report needs multiple kinds of information. So at a glance my approach with a "generic data map" in bugreport objects looks a bit more future-proof to me. Also a bit easier to set up, no need to deal with custom RTTI.

thanks for your reviews.

Added documentation to the newly introduced types: TaintData, TaintBugReport.

Thanks for this new check. Could you please link here results of this checker on som relevant open source projects?

I think we should document all clang analyzer options on this page (which is listed by clang -cc1 -analyzer-config-help"), where the section header should be the analyzer option name (e.g. consider-single-element-arrays-as-flexible-array-members).
I agree with @whisperity, that options, which affect the analysis should be described with code examples highlighting the changes the option causes in the analysis behaviour. (such as in case of consider-single-element-arrays-as-flexible-array-members, or widen-loops, unroll-loops etc.)

In D91948#2411058, @whisperity wrote:

Actually, while the explanation is understandable for me with additional knowledge about the representation... I think it would be useful to add the most simple example from the iterator checkers to the end of the document, how this whole thing ties together and how it is useful in a checker.

Since the analyzer cannot cannot model the size of the containers just yet (or precisely enough), what we are saying with this checker is to "always check the return value of the erase() function before use (for increment/decrement etc.) whether it is not past-end" .

If this is good to go, could you please commit this? Thanks!

In D69308#1727625, @NoQ wrote:

Another interesting problem that we forgot to mention on the open projects page is the modeling of C++17 bindings and decompositions: https://bugs.llvm.org/show_bug.cgi?id=43042

Also, in my opinion, out of all construction context problems mentioned on the open projects page, the NRVO problem is probably the easiest. It might as well be the least rewarding of them, but i think it is the friendliest possible problem to start with, as it doesn't force you to invent large new facilities.

In D69308#1727587, @NoQ wrote:

In D69308#1727108, @Szelethus wrote:

Would love to see this comment in its entirety on the open projects page :^)

I'd rather have a mention that @dkrupp is already working on this project, so that if somebody wanted to help out they could cooperate nicely.

Thanks for your comments @NoQ
I fixed them. Also added your implementation hints to the open projects page.

@aaron.ballman could you please check now? Thanks!

The patch is rebased to the latest master.

Thanks for the reviews!
Could you pls commit this for me?

@aaron.ballman could you please commit?
I don't have commit access. Thx.

Fixing minor capitalization issue and removing an extra newline.

I also analyzed openssl with the baseline and this version, but did not find any new warnings.
See:
http://codechecker-demo.eastus.cloudapp.azure.com/Default/#run=D66049_baseline&newcheck=D66049_improved&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=D66049_baseline_diff_D66049_improved

@Szelethus thanks for your review.
I fixed your suggestions.

Thanks @aaron.ballman and @alexfh for your review.
I fixed your findings.

Thanks for the comments @NoQ , all of them addressed.

Fix comments from @NoQ

@aaron.ballman 's comments are fixed.

Thanks Gabor for writing this.
I suggested some minor changes to the txt. Otherwise LGTM.

I guess this is a placeholder for the subpages of "User Manual" @ https://clang-analyzer.llvm.org, which will be ported in follow-up patches.

In D57858#1500635, @NoQ wrote:

In D57858#1499996, @dkrupp wrote:

Some alpha checkers are considerably more mature than others and are quite usable. In our experience, there are some users who are keen to run these checkers on their code and report back any false positives to us. So in this sense these are not "developer only" checkers. So I think we should let the users list them, read their descriptions and try them out. Some of them will come back with useful feedback as to how to improve them further.

What are such checkers currently? Like, the ones that aren't clearly "missing limbs" and that have somebody happy to address feedback sent against them?

Do you have a chance to call out to your users for testing the checker and actively request feedback, as @Szelethus did on the mailing list?

I feel that we could do some sort of "early access checkers" programme, but i believe this would require a more careful PR than just dumping a list of alpha checkers on our users' heads.

In D57858#1499996, @dkrupp wrote:

Some users would not care if the checker gives some more false positives than the "mature" checkers if they can catch some true positives with them.

Yeah, and these are pretty much the users we're trying to protect from themselves :)

In D57858#1499414, @Szelethus wrote:

In D57858#1498640, @NoQ wrote:

So, like, the global picture is as follows. In our case the Driver (i.e., --analyze) is not much more user facing than frontend flags. It's still fairly unusable directly, as our Static Analyzer is generally not a command-line tool. The real user-facing stuff is the GUIs such as scan-build or CodeChecker. These GUIs decide themselves on what options they want to expose. For instance, you have a right to decide that CodeChecker shouldn't support the aggressive mode of the move-checker and don't expose it as an option in your GUI. In this sense it's not really useful to provide a centralized -help of all user-facing options.

But it sounds as if you want to change this situation and provide a single source of truth on what are the user-facing options. Particular GUIs can still ignore them, but you don't want to hardcode flags in CodeChecker, but instead you want to rely on clang to provide the list of supported options for you and, as a side effect, for scan-build users (if you also add a scan-build help flag). I'm totally in favor of crystallizing such list of user-facing flags, and this patch sounds like a good step in that direction, assuming that non-user-facing options are hidden.

That describes my intention quite well :)

I'm still in favor of hiding alpha checkers (as they are for development only, much like debug flags; i'd recommend hiding them in the CodeChecker UI as well)

Now, why do we care about frontend/driver flags when they're unusable by definition? That's because we have a mental trauma after seeing a few powerusers actively explore those flags, observe that they don't work, and then tell everybody that the Analyzer is broken. So there's a threshold, based on a tiny but painful bit of practical experience, that says that documentation of developer-only features on llvm.org or in code comments is fine, but documentation printed by the released binary itself is not fine.

What you say sounds very reasonable. Still, I am kind of hesitant about hiding all alpha checkers: I initially intended to hide only are developer-only checkers (modeling, debug). I guess if we define alpha checkers (as you stated numerous times) as incomplete, under development, are missing half their limbs and crash if you look at them the wrong way, sure, they belong in the developer-only category. But checkers such as mine (UninitializedObjectChecker), for the longest time were very stable, have been enabled by default for our internal projects, despite only recently moving out of alpha.

Then agaaain, if we're that stubborn about alpha checkers, we could might as well dig them out of -analyzer-checker-help-hidden, and leave the rest there. Untangling what alpha checkers depend on one another could be solved by making yet another frontend flag that would display checker dependencies, which would be super easy since D54438, or create an -analyzer-checker-help-alpha flag that would display alpha, but not developer-only checkers. @dkrupp @o.gyorgy Do you have any feelings on this?

and we should probably automatically hide options of checker that are hidden.

Checker options are a different kind of animal entirely. I think we should definitely let the user fine-tune some modeling checkers, for instance, unix.DynamicMemoryModeling:Optimistic, despite us not really wanting to let anyone (even developers really) mess around whether unix.DynamicMemoryModeling should be enabled. While that specific option is, to put it nicely, a little esoteric, making some decisions the analyzer makes less conservative, or limiting state splits to help performance may be desirable in the future.

Let's move the rest of the discussion directly related to hiding checker options to D61839!

Thanks for your reviews!
I have fixed all your comments and rebased the patch to the latest master.

@dcoughlin I don't necessarily agree with you.
Let me explain why we think this feature is important.

Thanks @NoQ .
So I created a very simple main page with the table of contents only http://cc.elte.hu/clang-docs/docs/html/ClangStaticAnalyzer.html

Thanks for your comments. I fixed them all. I also added the handling of redundant sizeof() and alignof() operators on the way. Please check if OK now...

All comments fixed.

@dcoughlin @NoQ ping...

In D55255#1319784, @JonasToth wrote:

Committed, Thank you for the patch! Was there a bug-report for this issue? If yes can you please close it/reference?

Comments addressed. Please commit if looks good, I don't have commit rights.
Thanks.

new undef/defined testcase added

In D55125#1315335, @Szelethus wrote:

I see your point, but here's why I think it isn't a bug: I like to see macros as constexpr variables, and if I used those instead, I personally wouldn't like to get a warning just because they have the same value. In C, silencing such a warning isn't even really possible.

Another interesting thought, @donat.nagy's check works by comparing actual nodes of the AST, while this one would work with Lexer, but they almost want to do the same thing, the only difference is that the first checks whether two pieces of code are equivalent, and the second checks whether they are the same. Maybe it'd be worth to extract the logic into a simple areStmtsEqual(const Stmt *S1, const Stmt *S2, bool ShouldCompareLexically = false) function, that would do AST based comparison if the last param is set to false, and would use Lexer if set to true. After that, we could just add command line options to both of these checks, to leave it up to the user to choose in between them. Maybe folks who suffer from heavily macro-infested code would rather bear the obvious performance hit Lexer causes for little more precise warnings, and (for example) users of C++11 (because there are few excuses not to prefer constexpr there) could run in AST mode.

edit: I'm not actually all that sure about the performance hit. Just a guess.

But I'm just thinking aloud really.

In D55125#1314788, @JonasToth wrote:

In D55125#1314741, @Szelethus wrote:

@JonasToth this is the Lexer based expression equality check I talked about in D54757#1311516. The point of this patch is that the definition is a macro sure could be build dependent, but the macro name is not, so it wouldn't warn on the case I showed. What do you think?

Yes, this approach is possible.
IMHO it is still a bug/redudant if you do the same thing on both paths and warning on it makes the programmer aware of the fact. E.g. the macros might have been something different before, but a refactoring made them equal and resulted in this situation.

-clang-format applied
-clang:: namespace qualifiers removed

@dcoughlin could you please look into this?

-scanbuild and xcode pictures are included now
-intro text ("What is Static Analysis?" etc.) are put under the Introduction section
-Download section is created, but I am not sure how well was the this Mac OSX binary release section was maintained. Should users really download from this site or through a package manager instead?

making the diff full context.

Which means that for some calls we aren't even trying to make a CTU lookup.

Thanks @NoQ, we will take a look at it!

@NoQ do we need any more update to this patch? Thanks.

The patch has been updated.
Changes:

Would be interesting to extend this checker (maybe in an upcoming patch) to report on uninitialized members not only in constructors, but also copy constructors and move constructors.