[analyzer] Implement fine-grained suppressions via attributes

[NoQ]

Summary

At the moment, the only possible way to suppress analyzer warnings
is by cutting the code out with #ifdef. It is practically impossible
to do locally, so whole functions should stay not analyzed.

This patch introduces new attribute analyzer_suppress for these
purposes. Even without specifying checker IDs, it is far more
precise than the existing practice. It can be applied to statements
in question. Its proximity to the warning and the fact that this is
part of the code, drastically increases its chances to survive code
refactoring.

At the moment, new suppression attribute is limited to statements
and variable declarations. This is a deliberate design choice.
It is not clear and open for discussions how it should behave when
placed on function or class declarations.

First choice here is to suppress warnings within the declaration
and the body. This includes forward declarations, nested classes
and so on. This way is more obvious and probably more intuitive,
but creates a whole bunch of problems. First of all, it is a
rather crude solution. Suppressions in large scopes such as
functions and classes should be discouraged and that's why we
actually went to the statement level. The second problem is
with issues reported on declarations. The only way the user can
suppress such warnings is to put the annotation directly on
the declaration. However, this will suppress issues within the
body of this declaration, including nested functions and classes.
The paradox of this situation is that in this case the current
approach is actually counter-intuitive.

The second approach addresses these two problems. It states that
the attribute placed on the declaration suppresses only warnings
reported on this declaration, but not in its body. And while
this way seems reasonable when we talk about the stated problems,
for the regular user, who is not familiar with all this, it might
seem that suppressions on functions and classes are non-obvious
or simply ignored by the analyzer.

Additionally, further on, when we decide on stable checker
identifiers, we can implement finer approach, where the user can
specify those to suppress specific checks. Current code still
can be relevant as it gives "suppress all" option, which is usually
enough.

[vsavchenko]

Right now, I reused existing suppress attribute. However I did it in a rather unconventional manner. I allowed 0 arguments in one spelling and >1 in another, which seems odd.
I can see a couple other possible solutions here:

• Choose a "keyword" that would be used to suppress all warnings (e.g. "suppress(all)")
• Introduce two different suppress attributes (maybe even rename existing attribute to be GSLSuppress)

[jkorous]

Thanks for working on this!

[jkorous]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2932

Since IIUC a node can have multiple parents - does that mean we could end up traversing a node multiple times?
BTW do you have an example of a node that have multiple parents?

2961

There's a note for ASTContext::getParents:

"New callers should use ParentMapContext::getParents() directly."

https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a32d11844fdb82310b9059784fd4ceb6b

Shall we do that?

[vsavchenko]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2932

The vast majority of AST nodes have exactly one parent. As for the nodes that do have multiple parents, I only know about InitListExpr.
And yes, I think we can end up traversing a node multiple times, but no one seems to be bothered by that

2961

Sure!

[xazax.hun]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2922

What will this location return? In case of a leak warning, we might get a different instance of the same warning on separate paths. We usually pick the shortest path, but it can change when we slightly alter the source code. Maybe we want the user to put the suppression at the uniqueing location when such location exist? (The allocation in case of a leak warnings.) I think that would result in a better user experience and more robust suppression mechanism. An open question is how to educate the user about the correct way of suppression. Should we emit a suppress location to the user explicitly?

[aaron.ballman]

In D93110#2448587, @vsavchenko wrote:

Right now, I reused existing suppress attribute. However I did it in a rather unconventional manner. I allowed 0 arguments in one spelling and >1 in another, which seems odd.
I can see a couple other possible solutions here:

• Choose a "keyword" that would be used to suppress all warnings (e.g. "suppress(all)")
• Introduce two different suppress attributes (maybe even rename existing attribute to be GSLSuppress)

I don't think it's a problem that we use the same semantic attribute but with different syntax requirements for the arguments. The semantics for GSL suppression and Clang suppression are the same so it makes sense to me to use the same semantic attribute so we don't wind up checking isa<ThisAttr, ThatAttr>(D) everywhere. However, if it does start to get awkward, I think splitting the attributes into two different semantic attributes is preferable to using a keyword.

One usability concern I have with adding [[clang::suppress]] is that users are going to want to use it to suppress all the various kinds of diagnostics and not just clang static analyzer warnings. We get away with [[gsl::suppress]] because that's very specific to the C++ core guidelines, but we can't really hand-wave away the problem with [[clang::suppress]]. Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics? (This ignores some of the even harder diagnostics like ones that percolate back up from the backend, but we should probably keep those in mind as well.)

clang/include/clang/Basic/Attr.td

2799

The documentation will need to be updated for this. I have no idea if that may be a bit awkward because we're documenting two different suppression mechanisms (that do roughly the same thing).

clang/lib/Sema/SemaDeclAttr.cpp

5228–5235

The behavior here will be that [[gsl::suppress]] in C++11 mode and [[clang::suppress]] in C++11 mode require at least one argument, while [[clang::supress]] in C2x mode and __attribute__((suppress)) in all language modes allow any number of arguments (including zero). That doesn't seem like what you want. I think you need something more like:

// The [[gsl::suppress]] spelling requires at least one argument, but the GNU
// and [[clang::suppress]] spellings do not require any arguments.
if (AL.hasScope() && AL.getScopeName()->isStr("gsl") &&
    !checkAttributeAtLeastNumArgs(S, AL, 1))
  return;

clang/lib/Sema/SemaStmtAttr.cpp

55–63

Same issue here.

(In other news, it looks like this attribute could stand to be updated to be a DeclOrStmtAttr once D92800 lands -- that's not your problem to solve, more just an observation.)

69–70

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2910

The coding standard has us elide braces on single-line ifs (same comment applies elsewhere).

[vsavchenko]

clang/include/clang/Basic/Attr.td

2799

I decided not to change it yet because I was not sure that this is going to be the final solution.

clang/lib/Sema/SemaDeclAttr.cpp

5228–5235

That's much better, thanks!

clang/lib/Sema/SemaStmtAttr.cpp

55–63

This is actually something that I wanted to ask out of curiosity, how it works as a declaration attribute if it's declared as StmtAttr

69–70

Sure

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2910

Sure!

2922

Ah, leaks. I thought about those, and probably should've mentioned it here.
I think it is counter-intuitive to have separate locations for warnings themselves and for suppressions. Because this would be the first place where the user will try to put suppression. It is not obvious what to do with locations when we report that something didn't happen when it should've. But this group of checkers is relatively small, usually we do have a precise location where something bad happens. So, I believe that leaks should be addressed separately and not affect design for suppressions for the vast majority of checkers.
Speaking of leaks, RetainReleaseChecker reports leaks on the allocation and they can be suppressed that way.

[vsavchenko]

In D93110#2458613, @aaron.ballman wrote:

Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics?

Actually, this attribute is not used anywhere in the codebase (even in clang-tidy!). I think that it can be used for warning suppressions as well, it is more comfortable than pragmas IMO. However, I think that the problem of false positive or, for that matter, suppressions is much more visible with static analysis tools.

One usability concern I have with adding [[clang::suppress]] is that users are going to want to use it to suppress all the various kinds of diagnostics and not just clang static analyzer warnings.

Documentation will explicitly state that this mechanism is for the clang static analyzer and if users would like to use it for others things as well, that would prove that it is a good approach and can be fully supported in other parts of Clang.

Additionally, I was thinking about third parties developing their own static analysis tools using Clang as a parser for C/C++/Obj-C. They would probably also like to use this attribute. This is one reason why we probably shouldn't complain about unknown identifiers used with this attribute.

[aaron.ballman]

In D93110#2460375, @vsavchenko wrote:

In D93110#2458613, @aaron.ballman wrote:

Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics?

Actually, this attribute is not used anywhere in the codebase (even in clang-tidy!). I think that it can be used for warning suppressions as well, it is more comfortable than pragmas IMO. However, I think that the problem of false positive or, for that matter, suppressions is much more visible with static analysis tools.

Agreed that an attribute will feel like a more natural way to suppress diagnostics than using a pragma. However, use of the pragma in the wild suggests people are just as interested in suppressing frontend diagnostics as they are any other kind. I'd like to make sure that whatever we design can be used for all of these purposes. What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend. I also want to avoid confusing behavior. That's why I was wondering if you were considering the wider scope as part of this design.

For instance, given code like:

signed i = ...;
unsigned u = ...;
int *ptr = ...;
int what = ...;

int val = i < u ? *ptr : *ptr / what;

This code would have at least three different diagnostics -- a signed/unsigned comparison, a possible null dereference of ptr (twice), and a possible divide by zero. If the user tries to add [[clang::suppress]] to that statement, it won't suppress all of the diagnostics. Also, depending on how the suppression works, it may also be even more unintuitive because clang-tidy runs the clang static analyzer and the clang frontend, so the user may be able to suppress some warnings that look like they come from clang-tidy and not others. I think that's going to be confusing behavior for users and we don't want to live in that state for very long and we should be considering the bigger design of the feature (which may require an RFC given how many components are involved).

I'm not suggesting we have to roll out complete support for everything with the initial patch. I'm fine if the initial support only suppresses static analyzer warnings so long as there's a clear path forward for the other components and (hopefully) some promise of extending support to those within the next release or two. I don't want a repeat of what happened with [[gsl::suppress]] (where we added the attribute to Clang, didn't utilize it in tree, the C++ Core Guideline then changed the syntax for the attribute to be incompatible with what we provided, and we're left with something that's not particularly useful for anyone).

One usability concern I have with adding [[clang::suppress]] is that users are going to want to use it to suppress all the various kinds of diagnostics and not just clang static analyzer warnings.

Documentation will explicitly state that this mechanism is for the clang static analyzer and if users would like to use it for others things as well, that would prove that it is a good approach and can be fully supported in other parts of Clang.

I don't think documentation will be sufficient long-term. The attribute name is basically self-documenting in terms of what the user will expect it to do and it won't behave in the obvious way.

Additionally, I was thinking about third parties developing their own static analysis tools using Clang as a parser for C/C++/Obj-C. They would probably also like to use this attribute. This is one reason why we probably shouldn't complain about unknown identifiers used with this attribute.

That's a great point and is definitely something that should be considered as part of the wider concern.

[vsavchenko]

Squash all three commits, introduce new attribute and add documentation.

[NoQ]

What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

clang/include/clang/Basic/Attr.td

2805

[evil mode]
What's the expected behavior when the same variable (esp. global) has multiple redeclarations and only some of them wear the attribute?
[/evil mode]

I guess this mostly applies to globals and we don't have much checkers that would warn on globals (maybe some of those WebKit checkers by @jkorous may be affected).

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

598–599

Even though the attribute is specific to the static analyzer and from that point of view it's appropriate to keep this info in the BugReporter object (that will always be specific to the static analyzer), i believe we should ultimately move this into a higher-level entity in libAnalysis such as AnalysisConsumer. This would allow other tools such as clang-tidy to access that information when integrated into the static analyzer (eg., a-la D95403) and the user treats the entire conglomerate of tools as just "the static analyzer" and expects consistency across checks. That, of course, relies on our ability to look up attributes by source locations.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

31–32

So, like, i wish this could be replaced with bool isSuppressed(const PathDiagnostic &); so that to untie this from Static Analyzer-specific BugReport object.

There's no straightforward way to jump from BugReport to its specific PathDiagnostic, especially given that the same bug report may produce different PathDiagnostics for different consumers. But by the time suppressions kick in we can be certain that PathDiagnostic objects are fully constructed.

It's an interesting question whether different PathDiagnostics for the same bug report may interact with suppressions differently. For now this can't happen because all of them will have the same location and the same uniqueing location. We should obviously avoid such differences, probably protect ourselves against them with an assertion.

39

Depression fuel! :D

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

71

A recursive visitor would cause you to visit nested declarations (eg., lambdas, or methods of nested classes) multiple times. Is this necessary? A plain old ConstStmtVisitor that's taught to visit child statements recursively could easily avoid that. That's probably cheap though.

[vsavchenko]

Fix failing tests

[vsavchenko]

clang/include/clang/Basic/Attr.td

2805

You're right, there is no way to support local variables and not globals (except for more manual approach in Sema when we assign these attributes).

As of now, it will work on the declaration/definition where the bug is reported. It is not perfect, but I won't say that it is counter-intuitive either.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

31–32

In order to move fully from BugReport to PathDiagnostic, we can pre-map the whole TU with suppressed "areas" (in the same way we do it now for declarations with issues). If we do that, we need to decide where and when such initialization should take place.

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

71

I don't see how I can easily use StmtVisitors without A LOT OF boilerplate code that will essentially produce a RecursiveASTVisitor (it is very possible that I just don't know about some trick). I guess we can add some optimization if needed, but I never had performance problems with recursive visitors.

[NoQ]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

31–32

We still have access to decl-with-issue and uniqueing-decl in PathDiagnostic if that's what seems problematic to you (?)

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

71

The typical idiom is as follows:

void VisitChildren(const Stmt *S) {
  // Not a visitor callback - just a helper function.
  for (const Stmt *ChildS : S->children())
    if (ChildS)
      Visit(ChildS);
}

void VisitStmt(const Stmt *S) {
  // The default behavior that makes the visitor recursive over statements.
  VisitChildren(S); 
}

void VisitAttributedStmt(const AttributedStmt *AS) {
  VisitAttributedNode(AS);
  VisitChildren(AS); // This *optionally* goes into every overridden function.
}

Also you'll probably have to manually unwrap VisitVarDecl into VisitDeclStmt with a loop over decls. But generally i think that's relatively little boilerplate for the much better control it provides.

I never had performance problems with recursive visitors

Me too until i started doing clang-tidy --enable-check-profile :D So, like, i don't think that's going to be a real problem but kind of why not avoid it entirely.

[vsavchenko]

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

71

That makes sense, but it's not a no-brainer tradeoff IMO. It simply gives me confidence that I'm not losing any portion of the AST out of my sight.

[aaron.ballman]

In D93110#2529917, @NoQ wrote:

What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

[vsavchenko]

In D93110#2534690, @aaron.ballman wrote:

In D93110#2529917, @NoQ wrote:

What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

We can integrate these solutions together for clang-tidy and for clang static analyzer, it's not a problem at all. I would've used the existing Suppress attribute if it was without gsl part. As I mentioned in the summary, at the moment, we want to use it ONLY inside functions. I can see that SuppressAttr is not used anywhere in the whole, but I can't be sure that it's not used somewhere externally. So, I couldn't just reduce the scope of it.

[aaron.ballman]

In D93110#2560095, @vsavchenko wrote:

In D93110#2534690, @aaron.ballman wrote:

In D93110#2529917, @NoQ wrote:

What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

We can integrate these solutions together for clang-tidy and for clang static analyzer, it's not a problem at all. I would've used the existing Suppress attribute if it was without gsl part.

Sorry, I failed at being clear. Let me take another stab at it.

<ideally>
I don't want multiple unrelated semantic attributes to handle this concept. We can have a single semantic attribute with multiple ways of spelling it (e.g., [[gsl::suppress]] and [[clang::suppress]]) easily enough. I want to avoid code that looks like if (const auto *A = D->getAttr<SuppressAttr>()) { ... } else if (const auto *A = D->getAttr<SomeOtherSuppressAttr>()) { ... } because this causes maintenance problems where someone invariably forgets the less-used suppression attribute somewhere. (I would be fine if we had a common base class used by multiple suppression semantic attributes, if that was necessary to avoid this problem. )

I also don't want multiple attribute spellings for different parts of the toolchain. e.g., a user should not have to write [[clang::suppress(...)]] for a frontend suppression and [[tidy::suppress(...)]] for a tidy suppression, and [[csa::suppress(...)]] for a static analyzer suppression. This leaks implementation details out to users that they're not likely to have insights into anyway and it causes problems when we shift a diagnostic around or combine tools (like the static analyzer and tidy being able to run one another's checks).

I don't think it's an issue to have multiple attribute spellings to support different coding style guidelines or to be compatible with another compiler because users will be able to reason about those annotations more easily than which part of the toolchain a diagnostic comes from. So I don't think it's an issue to have [[gsl::suppress(...)]] and [[clang::suppress(...)]] as spellings so long as we wind up with a single semantic attribute interface under the hood.
</ideally>

As I mentioned in the summary, at the moment, we want to use it ONLY inside functions. I can see that SuppressAttr is not used anywhere in the whole, but I can't be sure that it's not used somewhere externally. So, I couldn't just reduce the scope of it.

We have the notion of attribute accessors to help with this sort of thing. It's reasonable to add an accessor to see whether the attribute was spelled with the gsl spelling, and if so, make alternative diagnostic decisions from it. e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L650

[vsavchenko]

Join 'suppress' and 'analyzer_suppress' attributes.

[vsavchenko]

Finally I had a chance to come back to this patch.
@aaron.ballman what do you think about it? I tried to address your notes and implemented both features under one attribute.

[vsavchenko]

@aaron.ballman a gentle ping

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

5271–5272

We may want to clarify that currently, this does not suppress all diagnostics and give an example where the FE diagnostic is not suppressed but a CSA one is. We may also want to mention that this is expected to be a temporary limitation?

5279

This attribute no longer exists.

5283–5284

Is this true in general, or just for leaks? e.g., does this also suppress on sources of taint rather than on uses of taint?

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

598–599

In addition to considering integration into clang-tidy, we should be considering integration into the Clang diagnostics engine as well. I don't think the attribute has to work in clang-tidy and clang proper as part of this initial patch, but we should have an idea on the path forward so we don't duplicate the user suppression list three times.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

10

15

Typo in the header guard.

clang/lib/Sema/SemaDeclAttr.cpp

5228–5235

Rather than == 0, this should be using something more like == SuppressAttr::CXX11_gsl_suppress (or whatever the enumeration spelling comes out as from tablegen).

5232

Should this also apply to FieldDecl? IndirectFieldDecl?

You mention in the summary that there are open design questions about what this should apply to. My thinking is that the second option is the most defensible one and is still possible to explain -- the attribute suppresses diagnostics on the line on which the attribute appears (with "line" being logical source line, not physical source line in a file). Then we can add an argument to the attribute in the future if we decide we need finer granularity (e.g., two diagnostics issued on the same line and you only want to suppress one diagnostic). Using the wider scope means we don't have any way to support the fine-grained control that I think is needed in practice. WDYT?

clang/lib/Sema/SemaStmtAttr.cpp

56

Same suggestion here as above for the == 0.

[NoQ]

Folks, I'm interested in finishing this patch so I guess I'll take over! I'll also ping discourse in a moment to talk more about the future of this feature.

clang/include/clang/Basic/AttrDocs.td

5271–5272

Clarified that frontend warnings aren't supported, more discussion below!

5283–5284

For now it's just leaks, but this isn't really about leaks. It's about how meaningful is the source location to which the warning is attached.

I rephrased this as "tools are allowed to do this at their discretion". It's probably better to leave this to documentation of individual tools or individual checkers, because the considerations are just so situational!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

598–599

I keep thinking about this and I'm quite worried that we'll have a hard time offering consistent user experience with clang frontend diagnostics. In particular, the attribute probably can't be used to suppress diagnostics emitted very early in the compilation pipeline, when neither the attribute object nor its respective Stmt are even constructed yet. It may be too late to suppress the diagnostic by the time we reach the closing curly brace of the CompoundStmt the attribute is attached to; I'll need to experiment further. This is what made #pragma clang diagnostic so nice for this purpose: it works even with preprocessor diagnostics!

So I don't believe that a solid path forward for frontend warnings *exists*. We'll probably have to draw an arbitrary line somewhere. It's somewhat likely that abandoning support for frontend warnings would be better than offering inconsistent experience that forces users to think about compilation phases. This probably deserves a deeper discussion backed up by some digging.

The current implementation assumes that the entire function is fully parsed, which is suitable for all static analysis purposes and for for analysis-based warnings purposes, but the majority of frontend warnings won't work this way and the implementation will need to be reworked. That said, the current implementation is tiny and the interface is trivial, so it shouldn't be too hard to re-do once we figure out how.

clang/lib/Sema/SemaDeclAttr.cpp

5232

Should this also apply to FieldDecl? IndirectFieldDecl?

As of today the reason why variables are special is that when we're attaching the attribute to a variable, what we're actually trying to say is that it's attached to the DeclStmt of the variable, as a statement. But the attribute syntax doesn't really offer much room to discriminate between the attributed DeclStmt and the attributed VarDecl, and it's actually nice to have a VarDecl granularity in DeclStmts that declare multiple variables, so we offer this special case. In this sense it doesn't make sense to add FieldDecl and IndirectFieldDecl to the list, given that their declaration is typically not a statement, but it might make sense to add support for TypedefDecl in the future, given that it can declare a VLA where the size is evaluated at runtime.

So this doesn't really have much to do with how exactly the attribute on a FieldDecl or say a FunctionDecl should work, at least not yet. I agree that confining the impact of the attribute to its lexical scope is probably the right thing to do.

[NoQ]

Rebase, address old review comments.

[NoQ]

One thing I want to change for the initial patch is (I'll hopefully get to this tomorrow): Currently the attribute ignores the string arguments (that were supposed to represent checker names). I think a better incremental approach would be to immediately start respecting the arguments, except that the analyzer doesn't actually "recognize" any of them, so the attribute stops working, stops suppressing the warning as soon as one or more arguments are passed. Then in follow-up patches we'll gradually teach it to recognize more and more arguments to get it to work again. The zero-argument syntax will continue to suppress all warnings, but the one-or-more-argument syntax should do what it's supposed to do from the start: suppress only the warnings that are recognized. This will also mean that we won't introduce any new warnings as we implement support for checker names, which could have happened if a user decided to pass their new poem as an argument for some reason.

[NoQ]

More Discourse discussion: https://discourse.llvm.org/t/going-forward-with-clang-suppress/75357

Also, folks, let me know if you want me to move to github 😅 it looks like this is not recommended but I'll be happy to do that if it sounds valuable. It'll also unblock me to send more follow-up pull requests on top of this patch - currently they won't apply cleanly.

clang/test/Analysis/suppression-attr.m

172–173

This FIXME is a regression from Valeriy's initial patch. It looks like the suppression used to work, but after rebase there's no way to suppress the warning at } directly, and I haven't yet figured out how it worked earlier. I'll take a look at what I can do about this in a separate patch; this is unfortunate but I don't think it's a blocker and it's likely the solution will be to make the static analyzer emit the warning earlier (eg. don't prune the NullStmt from the CFG if it carries an attribute).

[NoQ]

In D93110#4657691, @NoQ wrote:

I think a better incremental approach would be to immediately start respecting the arguments, except that the analyzer doesn't actually "recognize" any of them, so the attribute stops working, stops suppressing the warning as soon as one or more arguments are passed.

Done.

[NoQ]

Unforget tests for the newly added checker name stub.

[NoQ]

Better name for a parameter.

[NoQ]

Alright, I'll try to land this, as per the Discourse thread.

clang/include/clang/Basic/Attr.td

def SwiftAsyncError : InheritableAttr {
   let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Args = [EnumArgument<"Convention", "ConventionKind",
               ["none", "nonnull_error", "zero_argument", "nonzero_argument"],
               ["None", "NonNullError", "ZeroArgument", "NonZeroArgument"]>,
               UnsignedArgument<"HandlerParamIdx", /*opt=*/1>];
   let Documentation = [SwiftAsyncErrorDocs];
 }
 
-def Suppress : StmtAttr {
-  let Spellings = [CXX11<"gsl", "suppress">];
+def Suppress : DeclOrStmtAttr {
+  let Spellings = [CXX11<"gsl", "suppress">, Clang<"suppress">];
   let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
+  let Accessors = [Accessor<"isGSL", [CXX11<"gsl", "suppress">]>];
   let Documentation = [SuppressDocs];
 }
 
 def SysVABI : DeclOrTypeAttr {
   let Spellings = [GCC<"sysv_abi">];
 //  let Subjects = [Function, ObjCMethod];
   let Documentation = [Undocumented];
 }
 
 def ThisCall : DeclOrTypeAttr {
   let Spellings = [GCC<"thiscall">, CustomKeyword<"__thiscall">,
                    CustomKeyword<"_thiscall">];
 //  let Subjects = [Function, ObjCMethod];
   let Documentation = [ThisCallDocs];
 }

clang/include/clang/Basic/AttrDocs.td

 handler for this method, and the ``swift_async_error`` attribute specifies that
 the ``int`` parameter is the one that represents the error.
 }];
 }
 
 def SuppressDocs : Documentation {
   let Category = DocCatStmt;
   let Content = [{
-The ``[[gsl::suppress]]`` attribute suppresses specific
+The ``suppress`` attribute suppresses unwanted warnings coming from static
+analysis tools such as the Clang Static Analyzer. The tool will not report
+any issues in source code annotated with the attribute.
+
+The attribute cannot be used to suppress traditional Clang warnings, because
+many such warnings are emitted before the attribute is fully parsed.
+Consider using ``#pragma clang diagnostic`` to control such diagnostics,
+as described in `Controlling Diagnostics via Pragmas
+<https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas>`_.
+
+The ``suppress`` attribute can be placed on an individual statement in order to
+suppress warnings about undesirable behavior occurring at that statement:
+
+.. code-block:: c++
+
+  int foo() {
+    int *x = nullptr;
+    ...
+    [[clang::suppress]]
+    return *x;  // null pointer dereference warning suppressed here
+  }
+
+Putting the attribute on a compound statement suppresses all warnings in scope:
+
+.. code-block:: c++
+
+  int foo() {
+    [[clang::suppress]] {
+      int *x = nullptr;
+      ...
+      return *x;  // warnings suppressed in the entire scope
+    }
+  }
+
+Some static analysis warnings are accompanied by one or more notes, and the
+line of code against which the warning is emitted isn't necessarily the best
+for suppression purposes. In such cases the tools are allowed to implement
+additional ways to suppress specific warnings based on the attribute attached
+to a note location.
+
+For example, the Clang Static Analyzer suppresses memory leak warnings when
+the suppression attribute is placed at the allocation site (highlited by
+a "note: memory is allocated"), which may be different from the line of code
+at which the program "loses track" of the pointer (where the warning
+is ultimately emitted):
+
+.. code-block:: c
+
+  int bar1(bool coin_flip) {
+    __attribute__((suppress))
+    int *result = (int *)malloc(sizeof(int));
+    if (coin_flip)
+      return 1;  // warning about this leak path is suppressed
+
+    return *result;  // warning about this leak path is also suppressed
+  }
+
+  int bar2(bool coin_flip) {
+    int *result = (int *)malloc(sizeof(int));
+    if (coin_flip)
+      return 1;  // leak warning on this path NOT suppressed
+
+    __attribute__((suppress))
+    return *result;  // leak warning is suppressed only on this path
+  }
+
+
+When written as ``[[gsl::suppress]]``, this attribute suppresses specific
 clang-tidy diagnostics for rules of the `C++ Core Guidelines`_ in a portable
 way. The attribute can be attached to declarations, statements, and at
 namespace scope.
 
 .. code-block:: c++
 
   [[gsl::suppress("Rh-public")]]
   void f_() {

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
private:
   void FlushReport(BugReportEquivClass& EQ);
 
   /// The set of bug reports tracked by the BugReporter.
   llvm::FoldingSet<BugReportEquivClass> EQClasses;
 
   /// A vector of BugReports for tracking the allocated pointers and cleanup.
   std::vector<BugReportEquivClass *> EQClassesVector;
 
+  /// User-provided in-code suppressions.
+  BugSuppression UserSuppressions;
+
 public:
   BugReporter(BugReporterData &d);
   virtual ~BugReporter();
 
   /// Generate and flush diagnostics for all bug reports.
   void FlushReports();
 
   ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() {

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

+//===- BugSuppression.h - Suppression interface -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines BugSuppression, a simple interface class encapsulating
+//  all user provided in-code suppressions.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPRESSION_H
+#define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPRESSION_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace clang {
+class Decl;
+
+namespace ento {
+class BugReport;
+class PathDiagnosticLocation;
+
+class BugSuppression {
+public:
+  using DiagnosticIdentifierList = llvm::ArrayRef<llvm::StringRef>;
+
+  /// Return true if the given bug report was explicitly suppressed by the user.
+  bool isSuppressed(const BugReport &);
+
+  /// Return true if the bug reported at the given location was explicitly
+  /// suppressed by the user.
+  bool isSuppressed(const PathDiagnosticLocation &Location,
+                    const Decl *DeclWithIssue,
+                    DiagnosticIdentifierList DiagnosticIdentification);
+
+private:
+  // Overly pessimistic number, to be honest.
+  static constexpr unsigned EXPECTED_NUMBER_OF_SUPPRESSIONS = 8;
+  using CachedRanges =
+      llvm::SmallVector<SourceRange, EXPECTED_NUMBER_OF_SUPPRESSIONS>;
+
+  llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations;
+};
+
+} // end namespace ento
+} // end namespace clang
+
+#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPRESSION_H

clang/lib/Sema/SemaDeclAttr.cpp

case ParsedAttr::AT_M68kRTD:
     D->addAttr(::new (S.Context) M68kRTDAttr(S.Context, AL));
     return;
   default:
     llvm_unreachable("unexpected attribute kind");
   }
 }
 
 static void handleSuppressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (AL.getAttributeSpellingListIndex() == SuppressAttr::CXX11_gsl_suppress) {
+    // Suppression attribute with GSL spelling requires at least 1 argument.
     if (!AL.checkAtLeastNumArgs(S, 1))
       return;
+  } else if (!isa<VarDecl>(D)) {
+    // Analyzer suppression applies only to variables and statements.
+    S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type_str)
+        << AL << 0 << "variables and statements";
+    return;
+  }
 
   std::vector<StringRef> DiagnosticIdentifiers;
   for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
     StringRef RuleName;
 
     if (!S.checkStringLiteralArgumentAttr(AL, I, RuleName, nullptr))
       return;
 
-    // FIXME: Warn if the rule name is unknown. This is tricky because only
-    // clang-tidy knows about available rules.
     DiagnosticIdentifiers.push_back(RuleName);
   }
   D->addAttr(::new (S.Context)
                  SuppressAttr(S.Context, AL, DiagnosticIdentifiers.data(),
                               DiagnosticIdentifiers.size()));
 }
 
 static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL) {

clang/lib/Sema/SemaStmtAttr.cpp

if (!S.getLangOpts().CPlusPlus17 && A.isCXX11Attribute() &&
       !A.getScopeName())
     S.Diag(A.getLoc(), diag::ext_cxx17_attr) << A;
 
   FnScope->setHasFallthroughStmt();
   return ::new (S.Context) FallThroughAttr(S.Context, A);
 }
 
 static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
                                 SourceRange Range) {
+  if (A.getAttributeSpellingListIndex() == SuppressAttr::CXX11_gsl_suppress &&
+      A.getNumArgs() < 1) {
+    // Suppression attribute with GSL spelling requires at least 1 argument.
+    S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments) << A << 1;
+    return nullptr;
+  }
+
   std::vector<StringRef> DiagnosticIdentifiers;
   for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
     StringRef RuleName;
 
     if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, nullptr))
       return nullptr;
 
-    // FIXME: Warn if the rule name is unknown. This is tricky because only
-    // clang-tidy knows about available rules.
     DiagnosticIdentifiers.push_back(RuleName);
   }
 
   return ::new (S.Context) SuppressAttr(
       S.Context, A, DiagnosticIdentifiers.data(), DiagnosticIdentifiers.size());
 }
 
 static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
                                 SourceRange) {

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 //===- BugReporter.cpp - Generate PathDiagnostics for bugs ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines BugReporter, a utility class for generating
 //  PathDiagnostics.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CFGStmtMap.h"
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Analysis/ProgramPoint.h"
if (!S) {
     if (auto FE = P.getAs<FunctionExitPoint>()) {
       if (const ReturnStmt *RS = FE->getStmt())
         return PathDiagnosticLocation::createBegin(RS, SM, LC);
     }
     S = ErrorNode->getNextStmtForDiagnostics();
   }
 
   if (S) {
+    // Attributed statements usually have corrupted begin locations,
+    // it's OK to ignore attributes for our purposes and deal with
+    // the actual annotated statement.
+    if (const auto *AS = dyn_cast<AttributedStmt>(S))
+      S = AS->getSubStmt();
+
     // For member expressions, return the location of the '.' or '->'.
     if (const auto *ME = dyn_cast<MemberExpr>(S))
       return PathDiagnosticLocation::createMemberLoc(ME, SM);
 
     // For binary operators, return the location of the operator.
     if (const auto *B = dyn_cast<BinaryOperator>(S))
       return PathDiagnosticLocation::createOperatorLoc(B, SM);
 

 void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   bool ValidSourceLoc = R->getLocation().isValid();
   assert(ValidSourceLoc);
   // If we mess up in a release build, we'd still prefer to just drop the bug
   // instead of trying to go on.
   if (!ValidSourceLoc)
     return;
 
+  // If the user asked to suppress this report, we should skip it.
+  if (UserSuppressions.isSuppressed(*R))
+    return;
+
   // Compute the bug report's hash to determine its equivalence class.
   llvm::FoldingSetNodeID ID;
   R->Profile(ID);
 
   // Lookup the equivance class.  If there isn't one, create it.
   void *InsertPos;
   BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 
   if (!EQ) {
     EQ = new BugReportEquivClass(std::move(R));
     EQClasses.InsertNode(EQ, InsertPos);
     EQClassesVector.push_back(EQ);
   } else
     EQ->AddReport(std::move(R));
 }
 
 void PathSensitiveBugReporter::emitReport(std::unique_ptr<BugReport> R) {
   if (auto PR = dyn_cast<PathSensitiveBugReport>(R.get()))
     if (const ExplodedNode *E = PR->getErrorNode()) {
       // An error node must either be a sink or have a tag, otherwise
       // it could get reclaimed before the path diagnostic is created.
       assert((E->isSink() || E->getLocation().getTag()) &&
              "Error node must either be a sink or have a tag");
 
       const AnalysisDeclContext *DeclCtx =
           E->getLocationContext()->getAnalysisDeclContext();
       // The source of autosynthesized body can be handcrafted AST or a model
       // file. The locations from handcrafted ASTs have no valid source
       // locations and have to be discarded. Locations from model files should

 
 namespace {
 
 struct FRIEC_WLItem {
   const ExplodedNode *N;
   ExplodedNode::const_succ_iterator I, E;
 
   FRIEC_WLItem(const ExplodedNode *n)
       : N(n), I(N->succ_begin()), E(N->succ_end()) {}
 };
 
 } // namespace
 
 BugReport *PathSensitiveBugReporter::findReportInEquivalenceClass(
     BugReportEquivClass &EQ, SmallVectorImpl<BugReport *> &bugReports) {
   // If we don't need to suppress any of the nodes because they are
   // post-dominated by a sink, simply add all the nodes in the equivalence class

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

+//===- BugSuppression.cpp - Suppression interface -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+using Ranges = llvm::SmallVectorImpl<SourceRange>;
+
+inline bool hasSuppression(const Decl *D) {
+  // FIXME: Implement diagnostic identifier arguments
+  // (checker names, "hashtags").
+  if (const auto *Suppression = D->getAttr<SuppressAttr>())
+    return !Suppression->isGSL() &&
+           (Suppression->diagnosticIdentifiers().empty());
+  return false;
+}
+inline bool hasSuppression(const AttributedStmt *S) {
+  // FIXME: Implement diagnostic identifier arguments
+  // (checker names, "hashtags").
+  return llvm::any_of(S->getAttrs(), [](const Attr *A) {
+    const auto *Suppression = dyn_cast<SuppressAttr>(A);
+    return Suppression && !Suppression->isGSL() &&
+           (Suppression->diagnosticIdentifiers().empty());
+  });
+}
+
+template <class NodeType> inline SourceRange getRange(const NodeType *Node) {
+  return Node->getSourceRange();
+}
+template <> inline SourceRange getRange(const AttributedStmt *S) {
+  // Begin location for attributed statement node seems to be ALWAYS invalid.
+  //
+  // It is unlikely that we ever report any warnings on suppression
+  // attribute itself, but even if we do, we wouldn't want that warning
+  // to be suppressed by that same attribute.
+  //
+  // Long story short, we can use inner statement and it's not going to break
+  // anything.
+  return getRange(S->getSubStmt());
+}
+
+inline bool isLessOrEqual(SourceLocation LHS, SourceLocation RHS,
+                          const SourceManager &SM) {
+  // SourceManager::isBeforeInTranslationUnit tests for strict
+  // inequality, when we need a non-strict comparison (bug
+  // can be reported directly on the annotated note).
+  // For this reason, we use the following equivalence:
+  //
+  //   A <= B <==> !(B < A)
+  //
+  return !SM.isBeforeInTranslationUnit(RHS, LHS);
+}
+
+inline bool fullyContains(SourceRange Larger, SourceRange Smaller,
+                          const SourceManager &SM) {
+  // Essentially this means:
+  //
+  //   Larger.fullyContains(Smaller)
+  //
+  // However, that method has a very trivial implementation and couldn't
+  // compare regular locations and locations from macro expansions.
+  // We could've converted everything into regular locations as a solution,
+  // but the following solution seems to be the most bulletproof.
+  return isLessOrEqual(Larger.getBegin(), Smaller.getBegin(), SM) &&
+         isLessOrEqual(Smaller.getEnd(), Larger.getEnd(), SM);
+}
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
+  static void initialize(const Decl *D, Ranges &ToInit) {
+    CacheInitializer(ToInit).TraverseDecl(const_cast<Decl *>(D));
+  }
+
+  bool VisitVarDecl(VarDecl *VD) {
+    // Bug location could be somewhere in the init value of
+    // a freshly declared variable.  Even though it looks like the
+    // user applied attribute to a statement, it will apply to a
+    // variable declaration, and this is where we check for it.
+    return VisitAttributedNode(VD);
+  }
+
+  bool VisitAttributedStmt(AttributedStmt *AS) {
+    // When we apply attributes to statements, it actually creates
+    // a wrapper statement that only contains attributes and the wrapped
+    // statement.
+    return VisitAttributedNode(AS);
+  }
+
+private:
+  template <class NodeType> bool VisitAttributedNode(NodeType *Node) {
+    if (hasSuppression(Node)) {
+      // TODO: In the future, when we come up with good stable IDs for checkers
+      //       we can return a list of kinds to ignore, or all if no arguments
+      //       were provided.
+      addRange(getRange(Node));
+    }
+    // We should keep traversing AST.
+    return true;
+  }
+
+  void addRange(SourceRange R) {
+    if (R.isValid()) {
+      Result.push_back(R);
+    }
+  }
+
+  CacheInitializer(Ranges &R) : Result(R) {}
+  Ranges &Result;
+};
+
+} // end anonymous namespace
+
+// TODO: Introduce stable IDs for checkers and check for those here
+//       to be more specific.  Attribute without arguments should still
+//       be considered as "suppress all".
+//       It is already much finer granularity than what we have now
+//       (i.e. removing the whole function from the analysis).
+bool BugSuppression::isSuppressed(const BugReport &R) {
+  PathDiagnosticLocation Location = R.getLocation();
+  PathDiagnosticLocation UniqueingLocation = R.getUniqueingLocation();
+  const Decl *DeclWithIssue = R.getDeclWithIssue();
+
+  return isSuppressed(Location, DeclWithIssue, {}) ||
+         isSuppressed(UniqueingLocation, DeclWithIssue, {});
+}
+
+bool BugSuppression::isSuppressed(const PathDiagnosticLocation &Location,
+                                  const Decl *DeclWithIssue,
+                                  DiagnosticIdentifierList Hashtags) {
+  if (!Location.isValid() || DeclWithIssue == nullptr)
+    return false;
+
+  // While some warnings are attached to AST nodes (mostly path-sensitive
+  // checks), others are simply associated with a plain source location
+  // or range.  Figuring out the node based on locations can be tricky,
+  // so instead, we traverse the whole body of the declaration and gather
+  // information on ALL suppressions.  After that we can simply check if
+  // any of those suppressions affect the warning in question.
+  //
+  // Traversing AST of a function is not a heavy operation, but for
+  // large functions with a lot of bugs it can make a dent in performance.
+  // In order to avoid this scenario, we cache traversal results.
+  auto InsertionResult = CachedSuppressionLocations.insert(
+      std::make_pair(DeclWithIssue, CachedRanges{}));
+  Ranges &SuppressionRanges = InsertionResult.first->second;
+  if (InsertionResult.second) {
+    // We haven't checked this declaration for suppressions yet!
+    CacheInitializer::initialize(DeclWithIssue, SuppressionRanges);
+  }
+
+  SourceRange BugRange = Location.asRange();
+  const SourceManager &SM = Location.getManager();
+
+  return llvm::any_of(SuppressionRanges,
+                      [BugRange, &SM](SourceRange Suppression) {
+                        return fullyContains(Suppression, BugRange, SM);
+                      });
+}

clang/lib/StaticAnalyzer/Core/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
 )
 
 add_clang_library(clangStaticAnalyzerCore
   APSIntType.cpp
   AnalysisManager.cpp
   AnalyzerOptions.cpp
   BasicValueFactory.cpp
   BlockCounter.cpp
   BugReporter.cpp
   BugReporterVisitors.cpp
+  BugSuppression.cpp
   CallDescription.cpp
   CallEvent.cpp
   Checker.cpp
   CheckerContext.cpp
   CheckerHelpers.cpp
   CheckerManager.cpp
   CheckerRegistryData.cpp
   CommonBugCategories.cpp

clang/test/Analysis/suppression-attr-doc.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix \
+// RUN:                    -analyzer-disable-checker=core.uninitialized \
+// RUN:                    -verify %s
+
+// NOTE: These tests correspond to examples provided in documentation
+// of [[clang::suppress]]. If you break them intentionally, it's likely that
+// you need to update the documentation!
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+int foo_initial() {
+  int *x = nullptr;
+  return *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+int foo1() {
+  int *x = nullptr;
+  [[clang::suppress]]
+  return *x;  // null pointer dereference warning suppressed here
+}
+
+int foo2() {
+  [[clang::suppress]] {
+    int *x = nullptr;
+    return *x;  // null pointer dereference warning suppressed here
+  }
+}
+
+int bar_initial(bool coin_flip) {
+  int *result = (int *)malloc(sizeof(int));
+  if (coin_flip)
+    return 1; // There's no warning here YET, but it will show up if the other one is suppressed.
+
+  return *result;  // expected-warning{{Potential leak of memory pointed to by 'result'}}
+}
+
+int bar1(bool coin_flip) {
+  __attribute__((suppress))
+  int *result = (int *)malloc(sizeof(int));
+  if (coin_flip)
+    return 1;  // warning about this leak path is suppressed
+
+  return *result;  // warning about this leak path also suppressed
+}
+
+int bar2(bool coin_flip) {
+  int *result = (int *)malloc(sizeof(int));
+  if (coin_flip)
+    return 1;  // expected-warning{{Potential leak of memory pointed to by 'result'}}
+
+  __attribute__((suppress))
+  return *result;  // leak warning is suppressed only on this path
+}

clang/test/Analysis/suppression-attr.m

+// RUN: %clang_analyze_cc1 -fblocks \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.MissingSuperCall \
+// RUN:   -analyzer-checker=osx.cocoa.NSError \
+// RUN:   -analyzer-checker=osx.ObjCProperty \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=alpha.core.CastToStruct \
+// RUN:   -Wno-unused-value -Wno-objc-root-class -verify %s
+
+#define SUPPRESS __attribute__((suppress))
+#define SUPPRESS_SPECIFIC(...) __attribute__((suppress(__VA_ARGS__)))
+
+@protocol NSObject
+- (id)retain;
+- (oneway void)release;
+@end
+@interface NSObject <NSObject> {
+}
+- (id)init;
++ (id)alloc;
+@end
+typedef int NSInteger;
+typedef char BOOL;
+typedef struct _NSZone NSZone;
+@class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
+@protocol NSCopying
+- (id)copyWithZone:(NSZone *)zone;
+@end
+@protocol NSCoding
+- (void)encodeWithCoder:(NSCoder *)aCoder;
+@end
+@class NSDictionary;
+@interface NSError : NSObject <NSCopying, NSCoding> {
+}
++ (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
+@end
+
+@interface NSMutableString : NSObject
+@end
+
+typedef __typeof__(sizeof(int)) size_t;
+void *malloc(size_t);
+void free(void *);
+
+void dereference_1() {
+  int *x = 0;
+  *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+void dereference_suppression_1() {
+  int *x = 0;
+  SUPPRESS { *x; } // no-warning
+}
+
+void dereference_2() {
+  int *x = 0;
+  if (*x) { // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  }
+}
+
+void dereference_suppression_2() {
+  int *x = 0;
+  SUPPRESS if (*x) { // no-warning
+  }
+}
+
+void dereference_suppression_2a() {
+  int *x = 0;
+  // FIXME: Implement suppressing individual checkers.
+  SUPPRESS_SPECIFIC("core.NullDereference") if (*x) { // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  }
+}
+
+void dereference_suppression_2b() {
+  int *x = 0;
+  // This is not a MallocChecker issue so it shouldn't be suppressed. (Though the attribute
+  // doesn't really understand any of those arguments yet.)
+  SUPPRESS_SPECIFIC("unix.Malloc") if (*x) { // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  }
+}
+
+void dereference_3(int cond) {
+  int *x = 0;
+  if (cond) {
+    (*x)++; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  }
+}
+
+void dereference_suppression_3(int cond) {
+  int *x = 0;
+  SUPPRESS if (cond) {
+    (*x)++; // no-warning
+  }
+}
+
+void dereference_4() {
+  int *x = 0;
+  int y = *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+void dereference_suppression_4() {
+  int *x = 0;
+  SUPPRESS int y = *x; // no-warning
+}
+
+void dereference_5() {
+  int *x = 0;
+  int y = *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  int z = *x; // no-warning (duplicate)
+}
+
+void dereference_suppression_5_1() {
+  int *x = 0;
+  SUPPRESS int y = *x; // no-warning
+  int z = *x;          // no-warning (duplicate)
+}
+
+void dereference_suppression_5_2() {
+  int *x = 0;
+  int y = *x;          // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+  SUPPRESS int z = *x; // no-warning
+}
+
+void do_deref(int *y) {
+  *y = 1; // expected-warning{{Dereference of null pointer (loaded from variable 'y')}}
+}
+
+void dereference_interprocedural() {
+  int *x = 0;
+  do_deref(x);
+}
+
+void do_deref_suppressed(int *y) {
+  SUPPRESS *y = 1; // no-warning
+}
+
+void dereference_interprocedural_suppressed() {
+  int *x = 0;
+  do_deref_suppressed(x);
+}
+
+int malloc_leak_1() {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+  return *x; // expected-warning{{Potential leak of memory pointed to by 'x'}}
+}
+
+int malloc_leak_suppression_1_1() {
+  SUPPRESS int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+  return *x;
+}
+
+int malloc_leak_suppression_1_2() {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+  SUPPRESS return *x;
+}
+
+void malloc_leak_2() {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+
+void malloc_leak_suppression_2_1() {
+  SUPPRESS int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+}
+
+// TODO: reassess when we decide what to do with declaration annotations
+void malloc_leak_suppression_2_2() /* SUPPRESS */ {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+
+// TODO: reassess when we decide what to do with declaration annotations
+/* SUPPRESS */ void malloc_leak_suppression_2_3() {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+
+void malloc_leak_suppression_2_4(int cond) {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 42;
+  SUPPRESS;
+  // FIXME: The warning should be suppressed but dead symbol elimination
+  // happens too late.
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+
+void retain_release_leak_1() {
+  [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object of type 'NSMutableString *'}}
+}
+
+void retain_release_leak_suppression_1() {
+  SUPPRESS { [[NSMutableString alloc] init]; }
+}
+
+void retain_release_leak_2(int cond) {
+  id obj = [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object stored into 'obj'}}
+  if (cond) {
+    [obj release];
+  }
+}
+
+void retain_release_leak__suppression_2(int cond) {
+  SUPPRESS id obj = [[NSMutableString alloc] init];
+  if (cond) {
+    [obj release];
+  }
+}
+
+@interface UIResponder : NSObject {
+}
+- (char)resignFirstResponder;
+@end
+
+@interface Test : UIResponder {
+}
+@property(copy) NSMutableString *mutableStr;
+// expected-warning@-1 {{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@end
+@implementation Test
+
+- (BOOL)resignFirstResponder {
+  return 0;
+} // expected-warning {{The 'resignFirstResponder' instance method in UIResponder subclass 'Test' is missing a [super resignFirstResponder] call}}
+
+- (void)methodWhichMayFail:(NSError **)error {
+  // expected-warning@-1 {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
+}
+@end
+
+@interface TestSuppress : UIResponder {
+}
+// TODO: reassess when we decide what to do with declaration annotations
+@property(copy) /* SUPPRESS */ NSMutableString *mutableStr;
+// expected-warning@-1 {{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@end
+@implementation TestSuppress
+
+// TODO: reassess when we decide what to do with declaration annotations
+- (BOOL)resignFirstResponder /* SUPPRESS */ {
+  return 0;
+} // expected-warning {{The 'resignFirstResponder' instance method in UIResponder subclass 'TestSuppress' is missing a [super resignFirstResponder] call}}
+
+// TODO: reassess when we decide what to do with declaration annotations
+- (void)methodWhichMayFail:(NSError **)error /* SUPPRESS */ {
+  // expected-warning@-1 {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
+}
+@end
+
+struct AB {
+  int A, B;
+};
+
+struct ABC {
+  int A, B, C;
+};
+
+void ast_checker_1() {
+  struct AB Ab;
+  struct ABC *Abc;
+  Abc = (struct ABC *)&Ab; // expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
+}
+
+void ast_checker_suppress_1() {
+  struct AB Ab;
+  struct ABC *Abc;
+  SUPPRESS { Abc = (struct ABC *)&Ab; }
+}

clang/test/SemaCXX/attr-suppress.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[gsl::suppress("globally")]];
+
+namespace N {
+[[gsl::suppress("in-a-namespace")]];
+}
+
+[[gsl::suppress("readability-identifier-naming")]] void f_() {
+  int *p;
+  [[gsl::suppress("type", "bounds")]] {
+    p = reinterpret_cast<int *>(7);
+  }
+
+  [[gsl::suppress]] int x;       // expected-error {{'suppress' attribute takes at least 1 argument}}
+  [[gsl::suppress()]] int y;     // expected-error {{'suppress' attribute takes at least 1 argument}}
+  int [[gsl::suppress("r")]] z;  // expected-error {{'suppress' attribute cannot be applied to types}}
+  [[gsl::suppress(f_)]] float f; // expected-error {{expected string literal as argument of 'suppress' attribute}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+  int i;
+  float f;
+};
+
+[[clang::suppress]];
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+
+namespace N {
+[[clang::suppress("in-a-namespace")]];
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+} // namespace N
+
+[[clang::suppress]] int global = 42;
+
+[[clang::suppress]] void foo() {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  [[clang::suppress]] int *p;
+
+  [[clang::suppress]] int a = 0;           // no-warning
+  [[clang::suppress()]] int b = 1;         // no-warning
+  [[clang::suppress("a")]] int c = a + b;  // no-warning
+  [[clang::suppress("a", "b")]] b = c - a; // no-warning
+
+  [[clang::suppress("a", "b")]] if (b == 10) a += 4; // no-warning
+  [[clang::suppress]] while (true) {}                // no-warning
+  [[clang::suppress]] switch (a) {                   // no-warning
+  default:
+    c -= 10;
+  }
+
+  int [[clang::suppress("r")]] z;
+  // expected-error@-1 {{'suppress' attribute cannot be applied to types}}
+  [[clang::suppress(foo)]] float f;
+  // expected-error@-1 {{expected string literal as argument of 'suppress' attribute}}
+}
+
+class [[clang::suppress("type.1")]] V {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  int i;
+  float f;
+};

clang/test/SemaCXX/suppress.cpp

-// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
-
-[[gsl::suppress("globally")]];
-
-namespace N {
-  [[gsl::suppress("in-a-namespace")]];
-}
-
-[[gsl::suppress("readability-identifier-naming")]]
-void f_() {
-  int *p;
-  [[gsl::suppress("type", "bounds")]] {
-    p = reinterpret_cast<int *>(7);
-  }
-
-  [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
-  [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
-  int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
-  [[gsl::suppress(f_)]] float f; // expected-error {{expected string literal as argument of 'suppress' attribut}}
-}
-
-union [[gsl::suppress("type.1")]] U {
-  int i;
-  float f;
-};

clang/test/SemaObjC/attr-suppress.m

+// RUN: %clang_cc1 -fsyntax-only -fblocks %s -verify
+
+#define SUPPRESS1 __attribute__((suppress))
+#define SUPPRESS2(...) __attribute__((suppress(__VA_ARGS__)))
+
+SUPPRESS1 int global = 42;
+
+SUPPRESS1 void foo() {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  SUPPRESS1 int *p;
+
+  SUPPRESS1 int a = 0; // no-warning
+  SUPPRESS2()
+  int b = 1; // no-warning
+  SUPPRESS2("a")
+  int c = a + b;                     // no-warning
+  SUPPRESS2("a", "b") { b = c - a; } // no-warning
+
+  SUPPRESS2("a", "b")
+  if (b == 10)
+    a += 4;              // no-warning
+  SUPPRESS1 while (1) {} // no-warning
+  SUPPRESS1 switch (a) { // no-warning
+  default:
+    c -= 10;
+  }
+
+  // GNU-style attributes and C++11 attributes apply to different things when
+  // written like this.  GNU  attribute gets attached to the declaration, while
+  // C++11 attribute ends up on the type.
+  int SUPPRESS2("r") z;
+  SUPPRESS2(foo)
+  float f;
+  // expected-error@-2 {{expected string literal as argument of 'suppress' attribute}}
+}
+
+union SUPPRESS2("type.1") U {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  int i;
+  float f;
+};
+
+SUPPRESS1 @interface Test {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+}
+@property SUPPRESS2("prop") int *prop;
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+- (void)bar:(int)x SUPPRESS1;
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+@end

clang/www/analyzer/faq.html

int foo(int length) {
   for (int i = 0; i < length; i++)
     x += 1;
   return length/x;
 }
 </pre>
 
 <h4 id="suppress_issue" class="faq">Q: How can I suppress a specific analyzer warning?</h4>
 
-<p>There is currently no solid mechanism for suppressing an analyzer warning,
-although this is currently being investigated. When you encounter an analyzer
-bug/false positive, check if it's one of the issues discussed above or if the
-analyzer <a href = "annotations.html#custom_assertions" >annotations</a> can
-resolve the issue. Second, please <a href = "filing_bugs.html">report it</a> to
-help us improve user experience. As the last resort, consider using <tt>__clang_analyzer__</tt> macro
+<p>When you encounter an analyzer bug/false positive, check if it's one of the
+issues discussed above or if the analyzer
+<a href = "annotations.html#custom_assertions" >annotations</a> can
+resolve the issue by helping the static analyzer understand the code better.
+Second, please <a href = "filing_bugs.html">report it</a> to help us improve
+user experience.</p>
+
+<p>Sometimes there's really no "good" way to eliminate the issue. In such cases
+you can "silence" it directly by annotating the problematic line of code with
+the help of Clang attribute '<a href="https://clang.llvm.org/docs/AttributeReference.html#suppress">suppress</a>':
+
+<pre class="code_example">
+int foo() {
+  int *x = nullptr;
+  ...
+  <span class="code_highlight">[[clang::suppress]]</span> {
+    // all warnings in this scope are suppressed
+    int y = *x;
+  }
+
+  // null pointer dereference warning suppressed on the next line
+  <span class="code_highlight">[[clang::suppress]]</span>
+  return *x
+}
+
+int bar(bool coin_flip) {
+  // suppress all memory leak warnings about this allocation
+  <span class="code_highlight">[[clang::suppress]]</span>
+  int *result = (int *)malloc(sizeof(int));
+
+  if (coin_flip)
+    return 0;      // including this leak path
+
+  return *result;  // as well as this leak path
+}
+</pre>
+
+
+<p>You can also consider using <tt>__clang_analyzer__</tt> macro
 <a href = "faq.html#exclude_code" >described below</a>.</p>
 
 <h4 id="exclude_code" class="faq">Q: How can I selectively exclude code the analyzer examines?</h4>
 
 <p>When the static analyzer is using clang to parse source files, it implicitly
 defines the preprocessor macro <tt>__clang_analyzer__</tt>. One can use this
 macro to selectively exclude code the analyzer examines. Here is an example: