[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4747-4748
+The ``suppress`` attribute can be applied to variable declarations and statements
+to suppess warnings from the Clang Static Analyzer. The analyzer will not report
+any issues on the annotated constructs.
+
----------------
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?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4755
+    ...
+    [[clang::analyzer_suppress]] int y = *x; // no warning from the analyzer
+    ...
----------------
This attribute no longer exists.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4759-4760
+
+For leaks, suppressions can work both when placed on the statement where the
+issue is reported and on the allocation site.
+
----------------
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?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:592-593
 
+  /// User-provided in-code suppressions.
+  BugSuppression UserSuppressions;
+
----------------
NoQ wrote:
> 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.
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.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:9
+//
+//  This file defines BugSuppression, a simple interface class incapsulating
+//  all user provided in-code suppressions.
----------------



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:14
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H
+#define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H
----------------
Typo in the header guard.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4708
 static void handleSuppressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  if (!AL.checkAtLeastNumArgs(S, 1))
+  if (AL.getAttributeSpellingListIndex() == 0) {
+    // Suppression attribute with GSL spelling requires at least 1 argument.
----------------
Rather than `== 0`, this should be using something more like ` == SuppressAttr::CXX11_gsl_suppress` (or whatever the enumeration spelling comes out as from tablegen).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4712
+      return;
+  } else if (!isa<VarDecl>(D)) {
+    // Analyzer suppression applies only to variables and statements.
----------------
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?


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:55
                                 SourceRange Range) {
+  if (A.getAttributeSpellingListIndex() == 0 && A.getNumArgs() < 1) {
+    // Suppression attribute with GSL spelling requires at least 1 argument.
----------------
Same suggestion here as above for the `== 0`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93110/new/

https://reviews.llvm.org/D93110