I also have some very neat ideas how the split up should go, but I'd like to mature the idea in my head for just a bit longer.

Polite ping :)

Ping, @xazax.hun, any objections?

In the meanwhile we managed to figure out where the problem lays, so if you're interested, I'm going to document it here.

Ping

In D53069#1274554, @george.karpenkov wrote:

If we want to be serious about this page, it really has to be auto-generated (like clang-tidy one), but I understand that this is a larger undertaking.

In D51531#1296256, @NoQ wrote:

In D51531#1286110, @Szelethus wrote:

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

Oh, right. Silly me :)

• Added the discussed compatibility flag that's off by default, but is enabled by default by the driver.
• Now emitting errors instead of warnings.
• For the first time, introduce errors rather then asserts for invalid input.

In D54557#1301869, @NoQ wrote:

In D54557#1300671, @Szelethus wrote:

Nice catch, would you like to make an issue on their project or shall I?

I guess it can be an outright pull request :) I'll see if i have time for that soon, please feel free to take initiative><

I'll do the honors then. :)

In D54557#1301829, @Szelethus wrote:

Yup, there seems to be a desire to keep it around. Let's add an entry to the open projects maybe?

Mmm, what is the project here? What changes are still necessary?

In D54557#1300653, @NoQ wrote:

In D54557#1299899, @Szelethus wrote:

I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. Aggressive isn't Pedantic because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.

I only kept the option around because i was under an impression that i'm intruding into a checker that already has some happy users, probably breaking existing workflows. If this option is unnecessary, i'd be happy to remove it :)

Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near future) work on this particular checker.

In D54557#1300581, @NoQ wrote:

In D54557#1299899, @Szelethus wrote:

Whenever I compile Rtags, I get a bunch of move related warnings, I'm curious how this patch behaves on that project. I'll take a look.

Whoops, sry, i accidentally had a look because i misread your message as if you wanted me to take a look >.<

(IMPORTANT) Spoiler alert!

It finds one positive (and one duplicate of that one positive):

rtags-move-positive.html1 MB

Download

I believe this positive is a real bug, but we can still do better here. We find it as a use-after-move of a local variable, which is not a bug on its own, i.e. the user intended to empty and then re-use the storage and it's fine, this is not a usual kind of typo where the user uses the pre-move variable instead of the post-move variable. But the real bug here is that this local variable uses an std::string as a field under the hood, which is, well, not guaranteed to be empty after move, like all other STL objects: https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring

NOTE: As also mentioned in this stackoverflow thread, we also need to exclude smart pointers from the STL check because they don't end up in unspecified state, and see if there are other cornercases here.

OMG That is cool. :D That project was fishy. Nice catch, would you like to make an issue on their project or shall I?

Unfortunately, we don't find this bug as an STL use-after-move bug because inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's a combination of running out of budget and a specific feature that we have. By flipping a single -analyzer-config flag that represents that feature, we are able to find such bugs as STL bugs (when local variable bugs are disabled in the checker). We're still not able to find the original bug, most likely due to running out of budget (i didn't debug this further), but we can find it in a minimal example:

#include "rct/Rct.h"

void foo() {
  String S1;
  String S2 = std::move(S1);
  S1 += "asdfg"; // use-after-move of a std::string
}

Here's the report that we are able to obtain for this trivial code snippet, and you can look up the answer to the exercise in the collapsed run-line :)

rtags-move-positive-simplified.html162 KB

Download

Thanks! :) *throws confetti*

In D52730#1289989, @donat.nagy wrote:

Could someone with commit rights commit this patch (if it is acceptable)? I don't have commit rights myself.

In D54438#1297602, @NoQ wrote:

Hmm, makes sense. Maybe static bool BlahBlahChecker::isApplicable(const LangOpts &LO) or something like that.

Btw, maybe instead of default constructor and ->setup(Args) method we could stuff all of the setup(Args)'s arguments into the one-and-only constructor for the checker?

Alpha checkers, at least to developers, are clearly stating "Please finish me!", but a non-alpha, enabled-by-default checker with a flag that you'd never know about unless you looked at the source code (at least up until I fix these) sounds like it'll be forgotten like many other similar flags.

How about std::list<T>::splice?

Awesome! :D

I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. Aggressive isn't Pedantic because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.

LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there.

• Added more documentation
• Changed the nullpointer semantics as @xazax.hun pointed out
• Realize that macro arguments themselves can be macros, handle them too.
• Add a bunch more test. The above point actually fixed a wrong macro expansion is CALL_LAMBDA that somehow didn't get noticed. Gotta pay more attention when making the test files.

@george.karpenkov Matching macros is a very non-trivial job, how would you feel if we shipped this patch as-is, and maybe leave a TODO about adding macro asserts down the line?

Unfortunately, I found yet another corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this project, and I fear that I can't test this enough to be be 100% it does it's job perfectly. Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is unnerving to put it lightly, but doing changes within Preprocessor is most definitely out of my reach.

Unfortunately, I found yet another corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this project, and I fear that I can't test this enough to be be 100% it does it's job perfectly. Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is unnerving to put it lightly, but doing changes within Preprocessor is most definitely out of my reach.

Thanks you so much! One of the reasons why I love working on this is because the great feedback I get. I don't wanna seem annoyingly grateful, but it's been a very enjoyable experience so far.

Hmmm, shouldn't we add this to MemRegion's interface instead?

Here's how I'm planning to approach this issue:

• Split MallocChecker and CStringChecker up properly, introduce MallocCheckerBase and CStringCheckerBase. I'm a little unsure about how I'll do this, but I'll share my thoughts when I come up with something clever.
• Some checkers do not register themselves in all cases[1], which should probably be handled at a higher level, preferably in Checkers.td. Warnings could be emitted when a checker is explicitly enabled but will not be registered.
• Once all checkers in all cases properly register themselves, refactor this patch to introduce dependencies in between the subcheckers of MallocChecker and CStringChecker. This will imply that every registry function registers one, and only one checker.
• Move CheckerManager::registerChecker<T> out of the registry functions, rename them from register##CHECKERNAME to setup##CHECKERNAME, and supply the mutable checker object and the immutable CheckerManager. The downside is that all checkers will have to be default constructible, and their fields will have to be set up in setup##CHECKERNAME, but I see this as an acceptable tradeoff for the extra security.
  • This will make it impossible to affect other checkers. I'm a little unsure whether this is achievable considering how intertwined everything is in MallocChecker, but even if the supplied CheckerManager will have to non-const, getCurrentCheckerName and setCurrentCheckerName could be eliminated for sure.
• Properly document why CheckerRegistry and CheckerManager are separate entities, how are the tblgen files processed, how are dependencies handled, and so on.
• Rebase my local checker option-related branches, and celebrate. Unless another skeleton falls out of the closet. You never know I guess.

Allow me to disagree again -- the reason why CheckerRegistry and the entire checker registration process is a mess is that suboptimal solutions were added instead of making the system do well on the long term. This is completely fine, I am guilty of this myself, but now that this problem has been highlighted, it's time to get things right. Some clever refactoring is long overdue.

Actually, no. The main problem here is that InnerPointerChecker doesn't only want to register MallocBase, it needs to modify the checker object. In any case, either MallocChecker.cpp needs the definition of InnerPointerChecker, or vice versa. Sure, I could separate out the part that adds a new way to register dependencies, but it doesn't stop anyone from making the same mistake again, so what would be the point of that? I think it would be just a lot cleaner to split those checkers up properly, and completely replace the current system of registering checkers to stop anyone accidentally doing the same mistake again.

In D54438#1296239, @NoQ wrote:

Mm, i don't understand. I mean, what prevents you from cutting it off even earlier and completely omitting that part of the patch? Somebody will get to this later in order to see how exactly does the separation needs to be performed.

In D54438#1296155, @NoQ wrote:

Can we reduce this patch to simply introducing the dependency item in Checkers.td and using it in, like, one place (eg., MallocChecker/CStringChecker inter-op) and discuss the rest of the stuff later before we jump into it?

Restored the discussed note.

In D54401#1295832, @george.karpenkov wrote:

Also, remove diags::note_suggest_disabling_all_checkers.

Isn't that a separate revision? Given that removing existing options is often questionable, I'd much rather see this patch separated.

In D54429#1295838, @george.karpenkov wrote:

What do you propose we should do with other pages on the existing website? (OpenProjects / etc.)

Might as well make it a method.

I'll read through this as soon as possible, but a HUGE thank you for this. This is what the Static Analyzer desperately needed for years. This will trivialize documenting, so conversations about the inner workings of the analyzer will not be buried in many year old closed revisions.

Thanks for taking a look! ^-^

LGTM! Let's continue the conversation about coding standards somewhere else.

Also, remove diags::note_suggest_disabling_all_checkers.

Mainly my problem is that checker files can be large and hard to maneuver, some forward declare and document everything, some are a big bowl of spaghetti, and I think having a checklist of how it should be organized to keep uniformity and readability would be great.

In D52984#1294258, @NoQ wrote:

In D52984#1294233, @aaron.ballman wrote:

Personally, I think it's detrimental to the community for subprojects to come up with their own coding guidelines. My preference is for the static analyzer to come more in line with the rest of the project (over time, organically) in terms of style, terminology, diagnostic wording, etc. However, if the consensus is that we want a separate coding standard, I think it should be explicitly documented somewhere public and then maintained as part of the project.

I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid addTransition() hell - i guess we already have that, or how to register custom immutable maps, or how to implement checker dependencies or inter-checker APIs, or how much do we want to split modeling and checking into separate checkers, stuff like that.

Ping, @NoQ, do you insist on a global set?

I agree that it's aesthetically satisfying, but it is (1) inconvenient to write because that's the whole new syntax you need to memorize, i.e. all those <> and semicolons and (2) not cooperating when i want to look up checker name by source file (have to scan a few pages up to see the alias for the package and then jump multiple times to see what it expands to). So i'm still for ditching tblgen eventually :) But i definitely don't insist.

Thanks! This patch was the last for a while directly related to non-checker config options, I'm currently stuck on them as I unearthed countless bugs that I have to fix beforehand. (Did you know that clang unit tests do not run with check-clang-analysis?)

Ping

In D52984#1269850, @NoQ wrote:

• Warning and note messages should be clear and easy to understand, even if a bit long.
  • Messages should start with a capital letter (unlike Clang warnings!) and should not end with ..
  • Articles are usually omitted, eg. Dereference of a null pointer -> Dereference of null pointer.

I don't see this point in the list.

I personally find the registration process very error-prone, the current infrastructure around it makes it super easy to mess up in a way that won't be detected for months, so we probably need a better approach altogether. It shouldn't be possible to do any harm within a registry function, especially not in a way that affects other checkers.

Bad news, this approach doesn't work too well, and here's why: https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/CheckerRegistry.cpp#L115

• Moved initializer functions to CompilerInvocation.cpp, like every other Options-like class.
• The fields for options are no longer Optional
• Fixed the test files

Also, context is missing :)

We probably should also add an entry about some code conventions we use here, for example, the use of auto was debated recently when used with SVal::getAs, maybe something like this:

• As an LLVM subproject, the code in the Static Analyzer should too follow the https://llvm.org/docs/CodingStandards.html, but we do have some further restrictions/additions to that:
  • Prefer the usage of auto when using SVal::getAs, and const auto * when using MemRegion::getAs. The coding standard states that "Use auto if and only if it makes the code more readable or easier to maintain.", and even though SVal::getAs<T> returns with llvm::Optional<T>, which is not completely trivial, SVal is one of the most heavily used types in the Static Analyzer, and this convention makes the code significantly more readable.
  • Use llvm::SmallString and llvm::raw_svector_ostream to construct report messages. The coding standard explicitly doesn't forbid the use of the <sstream> library, but we do.
  • Do not forward declare or define static functions inside an anonymous namespace in checker files.
  • Document checkers on top of the file, rather then with comments before the checker class.
  • Checker files usually contain the checker class, the registration functions, and of course the actual implementation (usually several more functions and data structures) of the checker logic. To make the code more readable, keep the checker class on top of the file, forward declare all data structures and implementation functions below that, and put the registration function on the very bottom. The actual logic should be in between. For example:

//===----- MyChecker.cpp -----------------------------------------*- C++ -*-==//
//
//                     The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// MyChecker is a tool to show an example how a checker file should be
// organized.
//
//===----------------------------------------------------------------------===//

I have no other objections, looks great!

In D52790#1285039, @NoQ wrote:

Looks great, let's land?

I'll probably land it after part 5, in order to ease on rebasing.

Not sure if i already asked - am i understanding correctly that this is a "poor-man's" support for macro expansions for tools that read plists but do not support those new plist macro dictionaries yet?

Correct, same as notes-as-events.

I wonder how expansions of huge macros will look as events.

Quite funny, actually :D Think about assert, it's quite a mouthful. I'll post screenshots when I have more time.