[analyzer] Checker reviewer's checklist

[xazax.hun]

Summary

This is a first proposal to have a checklist for reviewing checkers.
Feel free to propose additional points or moving it around in the HTML document (or putting it somewhere else?)
I think this would come handy to catch some of the most common errors, omissions when reviewing checks.
For example, we tend to not update the list of checks at the homepage.

+ Added Devin for potential word smithing.

[Szelethus]

I love the idea. I got a couple more:

Is -analyzer-checker=core included in all RUN: lines?
Is the description in Checkers.td clear to a regular user?
Is Checkers.td and the CMakeLists file alphabetically sorted?

[xazax.hun]

• Added the ideas from Kristof.

[Szelethus]

LGTM! I can think of a couple more, like

• making sure that clang-format was run on the new files,
• not forgetting header guards,
• making sure that variables and functions only intended for use within an assert call are handled in a way that release build bots won't break on them ((void)Var),
• every virtual overridden method is marked as override (bots love breaking on that),

but these are general rules, not specific to the analyzer, even if they are easily forgotten and aren't emphasized enough elsewhere.

[NoQ]

www/analyzer/checker_dev_manual.html

822

I think we actually need two separate checklists:

• for common bugs (careful use of non-fatal error nodes, etc. - stuff we check for on every review) - "Common pitfalls when developing a checker" (?), also "Analyzer-specific coding standards" (?)
• for out-of-alpha requirements (all boilerplate present, warnings are clear to all users, evaluated on large codebases) - "What makes a good checker?" (?)

824

Most importantly, "If addTransition() with unspecified predecessor is ever executed after generateNonFatalErrorNode(), is the state split intended?"

825

You mean like forgetting to add a transition to the second possible state?

826

You mean suppress false positives with a visitor? Interestingly, right now there are almost no checkers that do this. But i suspect that there's no good reason for that, just trackNullOrUndefValue() covers most of the needs.

I believe that this is also the right way to suppress false positives that come from inlined functions in headers. Such as STL, which we currently prevent from being inlined entirely, which not only fixes many false positives, but also introduces some due to information loss. If we instead suppressed positives when we notice that the bug itself or certain interesting path events (potentially checker-specific) occur in headers, we could have had even less false positives and probably some new true positives.

[xazax.hun]

www/analyzer/checker_dev_manual.html

822

Do you think these two lists should be here, or would you prefer two move them to different parts of the document?

824

Is generateNonFatalErrorNode special? Two addTransition calls could have similar unintended effect, though I can imagine that being more rare.

825

My intention here is to check if the sinks are justified. Each sink result in coverage loss, so we do not really want to generate them for minor warnings, when we could continue the analysis in a meaningful manner.

826

I think this guideline is not followed at the moment, but I have the following vision: For each false positive report, there should be a way to rewrite the source code in a natural way to suppress that report.

E.g.: let's look at malloc checker:
If we take an infeasible path, we could add an assert to help suppress the report (though it is not always trivial, and this is not specific to the checker).
If we have a custom free function, we should be able to mark that function, so no memory leak is reported.
If we have a memory pool like thing, we should be able to communicate this to the checker somehow.

Or in case of conversion checker, we could check for implicit casts, and we could handle explicit casts as intended and do not report them. So false positives could be suppressed by making some implicit casts explicit.

[MTC]

Greate idea! If we can enrich this list, it will not only help the reviewer, but also help beginners, like me, avoid some pitfalls when developing a new checker.

I agree with NoQ to classify the lists. In addition to the two categories proposed by NoQ, there is another classof issues, see below, we don't have to take them into account.

• Develop a checker(proposed by NoQ). Like:
  1. Does it have to be in the path-sensitive manner?
  2. Should we move it into clang-tidy?
  3. Prefer ASTMatcher to StmtVisitor?
  4. Use CallDescription to get a stricter filter.
  5. Could we use checkPreCall() instead of checkPreStmt(const CallExpr *, )?
  6. Could we put this function model into StdLibraryFunctionsChecker.cpp?
• Make a good checke(proposed by NoQ).
• More general coding standards. Like:
  1. Use isa<> instead of dyn_cast<> if we don't need the casted result.
  2. Use dyn_cast_or_null instead of dyn_cast to enable the null-check.
  3. Use llvm::make_unique instead of std::make_unique because std::make_unique is not included in C++11.
  4. Avoid unnecessary null-check
  5. ... and so on.

The third list can be long, and maybe we don't need to take it into account, because developers will follow this standards whether they are developing checkers or contribute to other projects, like LLDB. We need to dig deeper into the analyzer-specific coding standards.

[george.karpenkov]

@Szelethus @xazax.hun Since you guys are looking into the website, do you know why the top bar has disappeared from all pages? (E.g. http://clang-analyzer.llvm.org/available_checks.html )

[Szelethus]

I noticed it too, it's already on my TODO list. Didn't look into the causes just yet though.

[NoQ]

In D52984#1260761, @george.karpenkov wrote:

@Szelethus @xazax.hun Since you guys are looking into the website, do you know why the top bar has disappeared from all pages? (E.g. http://clang-analyzer.llvm.org/available_checks.html )

The relevant reading here is https://bugs.llvm.org/show_bug.cgi?id=33811 I believe it's a server configuration error (good old "server-side includes" are disabled), and there's not much we can do other than inline the header into every page, which is horrible. Maybe some sort of javascript hack to load the headers from another URL and then inject them into the page might work.

[george.karpenkov]

inline the header into every page, which is horrible

is it though?

we probably also should be moving towards an auto-generated model anyway, if we want to e.g. automatically build sections of a website from the code.

[NoQ]

we probably also should be moving towards an auto-generated model anyway, if we want to e.g. automatically build sections of a website from the code.

This too will most likely require server maintainers to intervene, so i think fixing SSI is more realistic.

[george.karpenkov]

This too will most likely require server maintainers to intervene, so i think fixing SSI is more realistic.

Not really.
The way it is done e.g. for http://clang.llvm.org/docs/LibASTMatchersReference.html is the HTML files are in the repo, there is a script generating them from doxygen,
and the script has to be run manually. Still seems a lesser evil.

[NoQ]

is it though?

If we simply add a new menu point, it'll require us to modify a huge number of files. We might get away with good comments that explain the situation and making sure that all changes are done with sed, but i guess we'll still be stuck forever.

HTML files are in the repo, there is a script generating them from doxygen, and the script has to be run manually.

Yeah, i guess we could do that.

[NoQ]

My take on the out-of-alpha checklist:

• The checker should be evaluated on a large codebase in order to discover crashes and false positives specific to the checker. It should demonstrate the expected reasonably good results on it, with the stress on having as little false positives as possible.
  • The codebase should contain patterns that the checker checks. It should ideally also contain actual bugs, but it is fine if the checker was inspired by a one-of-a-kind devastating bug while there aren't many other instances of this bug around.

• 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.

• Static Analyzer's checker API saves you from a lot of work, but it still forces you into a certain amount of boilerplate code when it comes to managing entities specific to your checker:
  • Almost all path-sensitive checkers need their own BugReporterVisitor in order to highlight interesting events along the path - or at least a call to trackNullOrUndefValue. (need to wait for D52758)
    • For example, SimpleStreamChecker should highlight the event of opening the file when reporting a file descriptor leak.
  • If the checker tracks anything in the program state, it needs to implement the checkDeadSymbols callback to clean the state up.

• Unless there's a strong indication that the user is willing to opt-in to a stricter checking, obvious false positives that could be caused by the checker itself should be fixed. For example:
  • If a checker tracks state of mutable objects enumerated by symbols, escaping symbols into unfamiliar functions must invalidate the state - for example, through checkPointerEscape.
    • For example, SimpleStreamChecker should drop its knowledge about an open file descriptor when it is passed into a function that may potentially close the descriptor. At the same time, it is fine to not escape closed descriptors.
  • If the checker is intentionally leaving a room for false positives, i.e., it checks for a coding standard that isn't critical to follow when it comes to formal correctness of the program, then there should be a cheap explicit way to suppress the warnings when the violation of the coding standard is intentional.
  • Fuzzy matching of API function or type names is generally dangerous when it may lead to false positives, fine otherwise.
    • For example, a checker for LLVM isa<> API shouldn't match it as isa*, so that it didn't accidentally make assumptions on how isalpha works.
    • It is perfectly fine to have some fuzziness for initial experiments in alpha - that's a great way to figure out which APIs you want to cover.

My take on the everyday checklist:

• Checkers are encouraged to actively participate in the analysis by sharing its knowledge about the program state with the rest of the analyzer, but they should not be disrupting the analysis unnecessarily:
  • If a checker splits program state, this must be based on knowledge that the newly appearing branches are definitely possible and worth exploring from the user's perspective. Otherwise the state split should be delayed until there's an indication that one of the paths is taken, or one of the paths needs to be dropped entirely.
    • For example, it is fine to eagerly split paths while modeling isalpha(x) as long as x is constrained accordingly on each path. At the same time, it is not a good idea to split paths over the return value of printf() while modeling the call because nobody ever checks for errors in printf; at best, it'd just double the remaining analysis time.
    • Caution is advised when using CheckerContext::generateNonFatalErrorNode because it generates an independent transition, much like addTransition. It is easy to accidentally split paths while using it.
      • Ideally, try to structure the code so that it was obvious that every addTransition or generateNonFatalErrorNode (or sequence of such if the split is intended) is immediately followed by return from the checker callback.
  • Multiple implementations of evalCall in different checkers should not conflict.
  • When implementing evalAssume, the checker should always return a non-null state for either the true assumption or the false assumption (or both).
  • Checkers shall not mutate values of expressions, i.e. use the ProgramState::BindExpr API, unless they are fully responsible for computing the value. Under no circumstances should they change non-Unknown values of expressions.
    • Currently the only valid use case for this API in checkers is to model the return value in the evalCall callback.
    • If expression values are incorrect, ExprEngine needs to be fixed instead.

• The following checker code patterns are not wrong but very suspicious:
  • In State->getSVal(Region), Region is not necessarily a TypedValueRegion and the optional type argument is not specified.
    • It is likely that the checker may accidentally try to dereference a void pointer.
  • Checker logic depends on whether a certain value is a Loc or NonLoc.
    • It should be immediately obvious whether the SVal is a Loc or a NonLoc depending on the AST that is being checked.
    • Checking whether a value is Loc or Unknown/Undefined or whether the value is NonLoc or Unknown/Undefined is totally fine.
  • New symbols are constructed in the checker via direct calls to SymbolManager, unless they are of SymbolMetadata class tagged by the checker, or they represent newly created values such as the return value in evalCall.
    • For modeling arithmetic/bitwise/comparison operations, SValBuilder should be used.
  • Custom ProgramPointTags are created within the checker.
    • There is usually no good reason for a checker to chain multiple nodes together, because checkers aren't worklists.
  • A BugReporterVisitor does something different from if (N->getState()->get<Trait>(Key) != N->getFirstPred()->getState()->get<Trait>(Key)) emitSomeNote();.
    • That's the easiest and safest idiom to highlight the changes in the program state.
    • It is not necessary to re-hardcode all sorts of statements on which the checker reacts into the visitor.

• Use safe and convenient APIs!
  • Always use CheckerContext::generateErrorNode and CheckerContext::generateNonFatalErrorNode for emitting bug reports. Most importantly, never emit report against CheckerContext::getPredecessor.
  • Prefer checkPreCall and checkPostCall to checkPreStmt<CallExpr> and checkPostStmt<CallExpr>.
  • Use CallDescription to detect hardcoded API calls in the program.
  • Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E).

• Common sources of crashes:
  • CallEvent::getOriginExpr is nullable - for example, it returns null for an automatic destructor of a variable.
    • The same applies to all values generated while the call was modeled, eg. SymbolConjured::getStmt is nullable.
  • CallEvent::getDecl is nullable - for example, it returns null for a call of symbolic function pointer.
  • addTransition, generateSink, generateNonFatalErrorNode, generateErrorNode are nullable because you can transition to a node that you have already visited.
  • Methods of CallExpr/FunctionDecl/CallEvent that return arguments crash when the argument is out-of-bounds.
    • If you checked the function name, it doesn't mean that the function has the expected number of arguments!
      • Which is why you should use CallDescription.
  • Nullability of different entities within different kinds of symbols and regions is usually documented via assertions in their constructors.

In D52984#1258724, @MTC wrote:

• More general coding standards.

We have https://llvm.org/docs/CodingStandards.html but yeah, having Analyzer-specific coding standards is cool. I guess i listed a few items above.

www/analyzer/checker_dev_manual.html

824

generateNonFatalErrorNode() is special because it's too counter-intuitive and hard to remember that it is in fact equivalent to addTransition(), i.e. it isn't "the same thing as generateErrorNode() just sounds less scary" :)

This becomes really weird when there are multiple checks done in callees of the checker callback, only some of which are non-fatal, followed by addTransition() at the end of the callback. In this case this problem makes the code a lot more complex: you need to pass the current node around through all callees.

[NoQ]

Hmm, i guess i sent this out too early.

As per our long-lasting tradition, i forgot the point about the web page :\

Bullets about RUN-lines, Checkers.td descriptions and CMakeLists are also great for everyday use :)

[Szelethus]

One more thing.

From what I've seen, new checkers are in an overwhelming majority (again, from what I've seen) made by beginners, so I'd propose to include comments in checkers that are at least in part understandable by a beginner. I don't mean to make make 60% of the code grey, but there are a lot of unexplained non-trivial hackery in some.

[NoQ]

Yes. Not just for checkers, not just for beginners :)

[Szelethus]

I think @NoQ's list would be great to add! @dkrupp, as far as I'm aware, has some works in progress to avoid the usage of HTML, so let's put this on hold for a bit.

[aaron.ballman]

www/analyzer/checker_dev_manual.html

834

Do we want to add an item for "Is the help text for the checker a useful description of what the check does?" or something along those lines? I happened to be looking at the help text for checkers and some of it is... less than ideal ("Checks MPI code", etc).

[Szelethus]

www/analyzer/checker_dev_manual.html

834

Strongly agreed.

[NoQ]

Ok, so where do we put it? :)

I think we should put the everyday checklist into the checker developer manual under a title like "Making Your Checker Better" and put the out-of-alpha checklist somewhere into docs/analyzer as plain text (because that's not a sort of info you expect to find in the manual), though we could duplicate some points from there on the website (eg., "write visitors!").

[xazax.hun]

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.

Please let me know if I forgot something or the order of the list should be changed.

[Szelethus]

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.
//
//===----------------------------------------------------------------------===//

#include "ClangSACheckers.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"

namespace {

// Any documentation you'd like to put here should instead be put on top of the
// file, where the checker is described.
class MyChecker : public Checker<check::EndFunction> {
public:
  MyChecker() = default;
  void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
};

/// Helper class that is only here for this example.
class Helper {
public:
  /// Helps.
  void help();
};

} // end of anonymous namespace

/// Determines whether the problem is solvable.
static bool isProblemSolvable();

//===----------------------------------------------------------------------===//
// Methods for MyChecker.
//===----------------------------------------------------------------------===//

void MyChecker::checkEndFunction(const ReturnStmt *RS,
                                 CheckerContext &C) const {
  // TODO: Implement the logic.
}

//===----------------------------------------------------------------------===//
// Methods for Helper.
//===----------------------------------------------------------------------===//

void Helper::help() {
  // TODO: Actually help.
}

//===----------------------------------------------------------------------===//
// Utility functions.
//===----------------------------------------------------------------------===//

static bool isProblemSolvable() {
  return false;
}

void ento::registerMyChecker(CheckerManager &Mgr) {
  auto *Chk = Mgr.registerChecker<MyChecker>();
}

[Szelethus]

Also, context is missing :)

[Szelethus]

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.

Coding standard related items should probably in a different list, alongside the out-of-alpha items.

edit: What I meant with that last sentence is that this LGTM provided you add the quoted point. :)

www/analyzer/checker_dev_manual.html

719

Make sure the checker list is updated

720

ensure the description is clear even to non-developers

[NoQ]

Thx!! Tiny nits.

www/analyzer/checker_dev_manual.html

720

to non-developers

Good luck explaining use-after-move to my grandma :)

Like, i mean, probably to non-analyzer-developers?

722–723

...that explain the warning to the user better.

728

...should correctly handle...
Or: ...should conservatively assume that the program is correct when...

744

Not necessarily all values (if the call is inlined, it may actually happen that all values are concrete), but definitely some values.

758

I suspect that the comma after 'that' is unnecessary.

763–764

Hmm. Because we're actively bashing the developer in this section, i think we should be careful about being clear what's wrong and what's right and why, while keeping every bullet self-contained (so that it wasn't taken out of the context, i.e. "Bad things: 1. X, 2. ..." shouldn't be accidentally understood as "X").

Eg.,

Patterns that you should most likely avoid even if they're not technically wrong:
* `BugReporterVisitor` should most likely not match the AST of the current program point to decide when to emit a note. It is much easier to determine that by observing changes in the program state.

777

<tt>ProgramPointTag</tt>s (?)

781

their knowledge

789

<tt>x</tt>

809–812

Let's move this bullet to the top? It's kinda lonely here after all those huge sections with tons of sub-bullets.

[xazax.hun]

www/analyzer/checker_dev_manual.html

719

I think at some point we should decide if we prefer the term check or checker to refer to these things :) Clang Tidy clearly prefers check.

[Szelethus]

www/analyzer/checker_dev_manual.html

719

That is the distinction I'm aware of too: checkers in the Static Analyzer, checks in clang-tidy.

[xazax.hun]

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

I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one.

[xazax.hun]

www/analyzer/checker_dev_manual.html

719

My understanding is the following: we want users to use the term check, since that is more widespread and used by other (non-clang) tools as well. The term checker is something like a historical artifact in the developer community of the static analyzer. But if this is not the case, I am happy to change the terminology. But I do want to have some input from rest of the community too :)

[aaron.ballman]

In D52984#1294223, @xazax.hun wrote:

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

I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one.

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.

www/analyzer/checker_dev_manual.html

719

I grew up with the term "checker", but I feel like "check" may have won the war. I don't have a strong opinion here though.

[NoQ]

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.

www/analyzer/checker_dev_manual.html

719

We have the word "checker" all over the website, in option names, and, most importantly, in the "How to write a checker in 24 hours" video. I don't think we have much choice (:

[Szelethus]

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.

Indeed, I don't mean to change anything about the LLVM coding guideline, just add a couple Static Analyzer specific additional restrictions.

[Szelethus]

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 (alpha.llvm.Conventions), and I think having a checklist of how it should be organized to keep uniformity and readability would be great.

[xazax.hun]

• Use the term checker instead of check.

[Szelethus]

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

Can you please mark inlines as done?

[NoQ]

www/analyzer/checker_dev_manual.html

678

Probably Checker here as well.

681

I think a link to the checker list would be great here.

738–740

I think this problem isn't fully fixed. Eg., "In State->getSVal(Region), if Region is not known to be a TypedValueRegion and the optional type argument is not specified, the checker may accidentally try to dereference a void pointer", and i think most other bullets here should be re-phrased a bit.

772

A . is missing here.

[xazax.hun]

• Addressed further comments.

[NoQ]

Thanks!!~

I'm slow, but at least i admit it... Just made myself a fancier phabricator query so that not to forget about patches, so hopefully i won't forget about patches that often anymore :/

I mean, seriously, i'm very happy that we're all doing this sort of stuff together.

www/analyzer/checker_dev_manual.html

 </pre>
 
 <p>To dump AST of a method that the current <tt>ExplodedNode</tt> belongs
 to:</p>
 <pre class="code">
     (gdb) <b>p C.getPredecessor()->getCodeDecl().getBody()->dump()</b>
 </pre>
 
+<h2 id=links>Making Your Checker Better</h2>
+<ul>
+<li>User facing documentation is important for adoption! Make sure the <a href="/available_checks.html">checker list </a>is updated
+    at the homepage of the analyzer. Also ensure the description is clear to
+    non-analyzer-developers in <tt>Checkers.td</tt>.</li>
+<li>Warning and note messages should be clear and easy to understand, even if a bit long.</li>
+<ul>
+  <li>Messages should start with a capital letter (unlike Clang warnings!) and should not
+      end with <tt>.</tt>.</li>
+  <li>Articles are usually omitted, eg. <tt>Dereference of a null pointer</tt> ->
+      <tt>Dereference of null pointer</tt>.</li>
+  <li>Introduce <tt>BugReporterVisitor</tt>s to emit additional notes that explain the warning
+      to the user better. There are some existing visitors that might be useful for your check,
+      e.g. <tt>trackNullOrUndefValue</tt>. For example, SimpleStreamChecker should highlight
+      the event of opening the file when reporting a file descriptor leak.</li>
+</ul>
+<li>If the check tracks anything in the program state, it needs to implement the
+    <tt>checkDeadSymbols</tt>callback to clean the state up.</li>
+<li>The check should conservatively assume that the program is correct when a tracked symbol
+    is passed to a function that is unknown to the analyzer.
+    <tt>checkPointerEscape</tt> callback could help you handle that case.</li>
+<li>Use safe and convenient APIs!</li>
+<ul>
+  <li>Always use <tt>CheckerContext::generateErrorNode</tt> and
+    <tt>CheckerContext::generateNonFatalErrorNode</tt> for emitting bug reports.
+    Most importantly, never emit report against <tt>CheckerContext::getPredecessor</tt>.</li>
+  <li>Prefer <tt>checkPreCall</tt> and <tt>checkPostCall</tt> to
+    <tt>checkPreStmt&lt;CallExpr&gt;</tt> and <tt>checkPostStmt&lt;CallExpr&gt;</tt>.</li>
+  <li>Use <tt>CallDescription</tt> to detect hardcoded API calls in the program.</li>
+  <li>Simplify <tt>C.getState()->getSVal(E, C.getLocationContext())</tt> to <tt>C.getSVal(E)</tt>.</li>
+</ul>
+<li>Common sources of crashes:</li>
+<ul>
+  <li><tt>CallEvent::getOriginExpr</tt> is nullable - for example, it returns null for an
+    automatic destructor of a variable. The same applies to some values generated while the
+    call was modeled, eg. <tt>SymbolConjured::getStmt</tt> is nullable.</li>
+  <li><tt>CallEvent::getDecl</tt> is nullable - for example, it returns null for a
+      call of symbolic function pointer.</li>
+  <li><tt>addTransition</tt>, <tt>generateSink</tt>, <tt>generateNonFatalErrorNode</tt>,
+    <tt>generateErrorNode</tt> are nullable because you can transition to a node that you have already visited.</li>
+  <li>Methods of <tt>CallExpr</tt>/<tt>FunctionDecl</tt>/<tt>CallEvent</tt> that
+    return arguments crash when the argument is out-of-bounds. If you checked the function name,
+    it doesn't mean that the function has the expected number of arguments!
+    Which is why you should use <tt>CallDescription</tt>.</li>
+  <li>Nullability of different entities within different kinds of symbols and regions is usually
+      documented via assertions in their constructors.</li>
+  <li><tt>NamedDecl::getName</tt> will fail if the name of the declaration is not a single token,
+    e.g. for destructors. You could use <tt>NamedDecl::getNameAsString</tt> for those cases.
+    Note that this method is much slower and should be used sparringly, e.g. only when generating reports
+    but not during analysis.</li>
+  <li>Is <tt>-analyzer-checker=core</tt> included in all test <tt>RUN:</tt> lines? It was never supported
+    to run the analyzer with the core checks disabled. It might cause unexpected behavior and
+    crashes. You should do all your testing with the core checks enabled.</li>
+</ul>
+</ul>
+<li>Patterns that you should most likely avoid even if they're not technically wrong:</li>
+<ul>
+  <li><tt>BugReporterVisitor</tt> should most likely not match the AST of the current program point
+      to decide when to emit a note. It is much easier to determine that by observing changes in
+      the program state.</li>
+  <li>In <tt>State->getSVal(Region)</tt>, if <tt>Region</tt> is not known to be a <tt>TypedValueRegion</tt>
+      and the optional type argument is not specified, the checker may accidentally try to dereference a
+      void pointer.</li>
+  <li>Checker logic should not depend on whether a certain value is a <tt>Loc</tt> or <tt>NonLoc</tt>.
+    It should be immediately obvious whether the <tt>SVal</tt> is a <tt>Loc</tt> or a
+    <tt>NonLoc</tt> depending on the AST that is being checked. Checking whether a value
+    is <tt>Loc</tt> or <tt>Unknown</tt>/<tt>Undefined</tt> or whether the value is
+    <tt>NonLoc</tt> or <tt>Unknown</tt>/<tt>Undefined</tt> is totally fine.</li>
+  <li>New symbols should not be constructed in the checker via direct calls to <tt>SymbolManager</tt>,
+    unless they are of <tt>SymbolMetadata</tt> class tagged by the checker,
+    or they represent newly created values such as the return value in <tt>evalCall</tt>.
+    For modeling arithmetic/bitwise/comparison operations, <tt>SValBuilder</tt> should be used.</li>
+  <li>Custom <tt>ProgramPointTag</tt>s should not be created within the checker. There is usually
+    no good reason for a checker to chain multiple nodes together, because checkers aren't worklists.</li>
+</ul>
+<li>Checkers are encouraged to actively participate in the analysis by sharing
+  their knowledge about the program state with the rest of the analyzer,
+  but they should not be disrupting the analysis unnecessarily:</li>
+<ul>
+  <li>If a checker splits program state, this must be based on knowledge that
+    the newly appearing branches are definitely possible and worth exploring
+    from the user's perspective. Otherwise the state split should be delayed
+    until there's an indication that one of the paths is taken, or one of the
+    paths needs to be dropped entirely. For example, it is fine to eagerly split
+    paths while modeling <tt>isalpha(x)</tt> as long as <tt>x</tt> is constrained accordingly on
+    each path. At the same time, it is not a good idea to split paths over the
+    return value of <tt>printf()</tt> while modeling the call because nobody ever checks
+    for errors in <tt>printf</tt>; at best, it'd just double the remaining analysis time.
+  </li>
+  <li>Caution is advised when using <tt>CheckerContext::generateNonFatalErrorNode</tt>
+    because it generates an independent transition, much like <tt>addTransition</tt>.
+    It is easy to accidentally split paths while using it. Ideally, try to
+    structure the code so that it was obvious that every <tt>addTransition</tt> or
+    <tt>generateNonFatalErrorNode</tt> (or sequence of such if the split is intended) is
+    immediately followed by return from the checker callback.</li>
+  <li>Multiple implementations of <tt>evalCall</tt> in different checkers should not conflict.</li>
+  <li>When implementing <tt>evalAssume</tt>, the checker should always return a non-null state
+      for either the true assumption or the false assumption (or both).</li>
+  <li>Checkers shall not mutate values of expressions, i.e. use the <tt>ProgramState::BindExpr</tt> API,
+    unless they are fully responsible for computing the value.
+    Under no circumstances should they change non-<tt>Unknown</tt> values of expressions.
+    Currently the only valid use case for this API in checkers is to model the return value in the <tt>evalCall</tt> callback.
+    If expression values are incorrect, <tt>ExprEngine</tt> needs to be fixed instead.</li>
+</ul>
+
 <h2 id=additioninformation>Additional Sources of Information</h2>
 
 Here are some additional resources that are useful when working on the Clang
 Static Analyzer:
 
 <ul>
 <li><a href="http://lcs.ios.ac.cn/~xuzb/canalyze/memmodel.pdf">Xu, Zhongxing &
 Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for Static Analysis of C
 Programs.</a></li>
 <li><a href="https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/README.txt">
 The Clang Static Analyzer README</a></li>
 <li><a href="https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt">
 Documentation for how the Store works</a></li>
 <li><a href="https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/IPA.txt">
 Documentation about inlining</a></li>
 <li> The "Building a Checker in 24 hours" presentation given at the <a
 href="http://llvm.org/devmtg/2012-11">November 2012 LLVM Developer's
 meeting</a>. Describes the construction of SimpleStreamChecker. <a
 href="http://llvm.org/devmtg/2012-11/Zaks-Rose-Checker24Hours.pdf">Slides</a>
 and <a
 href="https://youtu.be/kdxlsP5QVPw">video</a>
 are available.</li>
 <li>
 <a href="https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf">
 Artem Degrachev: Clang Static Analyzer: A Checker Developer's Guide
 </a> (reading the previous items first might be a good idea)</li>
 <li>The list of <a href="implicit_checks.html">Implicit Checkers</a></li>
 <li> <a href="http://clang.llvm.org/doxygen">Clang doxygen</a>. Contains
 up-to-date documentation about the APIs available in Clang. Relevant entries
 have been linked throughout this page. Also of use is the
 <a href="http://llvm.org/doxygen">LLVM doxygen</a>, when dealing with classes
 from LLVM.</li>
 <li> The <a href="http://lists.llvm.org/mailman/listinfo/cfe-dev">
 cfe-dev mailing list</a>. This is the primary mailing list used for
 discussion of Clang development (including static code analysis). The
 <a href="http://lists.llvm.org/pipermail/cfe-dev">archive</a> also contains
 a lot of information.</li>
 </ul>
 
 </div>
 </div>
 </body>
 </html>