[analyzer] Check the checker name, rather than the ProgramPointTag when silencing a checker

[Szelethus]

Summary

The program point created by the checker, even if it is an error node, might not be the same as the name under which the report is emitted. In the picture shown below, two tags are placed, one after the call (registering a new symbol as dynamically allocated with the tag unix.DynamicMemoryModeling), and one after the end of the call (when the symbol is leaked with the tag MallocChecker : DeadSymbolsLeak):

Whether its a good idea to create error nodes where the tag and the checker name is different -- I suppose not really, but it doesn't bother me much yet. Could add an assert later down the line.

// Generate leak node.
ExplodedNode *N = C.getPredecessor();
if (!Errors.empty()) {
  // Would it be more elegant to create the emitting checkers tag here? Mind that this is a checker that houses many others.
  static CheckerProgramPointTag Tag("MallocChecker", "DeadSymbolsLeak");
  N = C.generateNonFatalErrorNode(C.getState(), &Tag);
  if (N) {
    for (SmallVectorImpl<SymbolRef>::iterator
         I = Errors.begin(), E = Errors.end(); I != E; ++I) {
      HandleLeak(*I, N, C);
    }
  }
}

[Szelethus]

While I still see checker silencing as a solution to a problem that shouldn't exist, I'm unsure about the amount of tedious and invasive work required to make the underlying infrastructure elegant just for the sake of it being elegant. Most notably, checker silencing solves the problem where a checker finds fatal errors -- disabling such a checker also gets rid of the sink node, which makes the entire analysis behave differently. This is desirable, as explained by @a.sidorin in https://lists.llvm.org/pipermail/cfe-dev/2019-August/063135.html. Although I still have concerns about the user interface if we promoted checker silencing to be user facing, it might be worth investigating a bit further.

[NoQ]

Oof. Accurate!

// Would it be more elegant to create the emitting checkers tag here? Mind that this is a checker that houses many others.

I think the checkers should have a right to attach custom tags to their nodes. This right guarantees that they can create multiple different nodes with the same state (eg., the error state) and point (eg., the error point). Not sure why would they need it but I guess in some cases it may be easier for the checker to generate an extra node than to pass existing node around, as the existing node may be next to impossible to retrieve given that addTransition() with the same state will simply return null if the node is already there. So, like, the same reason why generally we allow checkers to chain nodes instead of forcing them to make all changes in a single transition.

[vsavchenko]

Oh, I'm late to the party!
Glad to see you back @Szelethus !

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

: BugReporterContext(BRC), BugPath(std::move(BugPath)), R(r),
       VisitorsDiagnostics(std::move(VisitorsDiagnostics)) {}
 
 std::unique_ptr<PathDiagnostic>
 PathDiagnosticBuilder::generate(const PathDiagnosticConsumer *PDC) const {
   PathDiagnosticConstruct Construct(PDC, ErrorNode, R);
 
   const SourceManager &SM = getSourceManager();
   const AnalyzerOptions &Opts = getAnalyzerOptions();
-  StringRef ErrorTag = ErrorNode->getLocation().getTag()->getTagDescription();
 
   // See whether we need to silence the checker/package.
   // FIXME: This will not work if the report was emitted with an incorrect tag.
   for (const std::string &CheckerOrPackage : Opts.SilencedCheckersAndPackages) {
-    if (ErrorTag.startswith(CheckerOrPackage))
+    if (R->getBugType().getCheckerName().startswith(CheckerOrPackage))
       return nullptr;
   }
 
   if (!PDC->shouldGenerateDiagnostics())
     return generateEmptyDiagnosticForReport(R, getSourceManager());
 
   // Construct the final (warning) event for the bug report.
   auto EndNotes = VisitorsDiagnostics->find(ErrorNode);

clang/test/Analysis/malloc.cpp

-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete -analyzer-store=region -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-unknown-linux-gnu -w -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete -analyzer-store=region -verify %s
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete -analyzer-store=region -DTEST_INLINABLE_ALLOCATORS -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-unknown-linux-gnu -w -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete -analyzer-store=region -DTEST_INLINABLE_ALLOCATORS -verify %s
+// RUN: %clang_analyze_cc1 -w -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete
+
+// RUN: %clang_analyze_cc1 -w -verify %s \
+// RUN:   -triple i386-unknown-linux-gnu \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete
+
+// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete
+
+// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
+// RUN:   -triple i386-unknown-linux-gnu \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
 void *realloc(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size);

clang/test/Analysis/silence-checkers-malloc.cpp

+// RUN: %clang_analyze_cc1 -verify="no-silence" %s \
+// RUN:   -triple i386-unknown-linux-gnu \
+// RUN:   -analyzer-checker=core,apiModeling \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete
+
+// RUN: %clang_analyze_cc1 -verify="unix-silenced" %s \
+// RUN:   -triple i386-unknown-linux-gnu \
+// RUN:   -analyzer-checker=core,apiModeling \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=cplusplus.NewDelete\
+// RUN:   -analyzer-config silence-checkers="unix"
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void free(void *);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t nmemb, size_t size);
+char *strdup(const char *s);
+
+void checkThatMallocCheckerIsRunning() {
+  malloc(4);
+} // no-silence-warning{{Potential memory leak [unix.Malloc]}}
+
+int const_ptr_and_callback_def_param_null(int, const char *, int n, void (*)(void *) = 0);
+void r11160612_no_callback() {
+  char *x = (char *)malloc(12);
+  const_ptr_and_callback_def_param_null(0, x, 12);
+} // no-silence-warning{{Potential leak of memory pointed to by 'x' [unix.Malloc]}}
+
+#define ZERO_SIZE_PTR ((void *)16)
+
+void test_delete_ZERO_SIZE_PTR() {
+  int *Ptr = (int *)ZERO_SIZE_PTR;
+  // ZERO_SIZE_PTR is specially handled but only for malloc family
+  delete Ptr; // no-silence-warning{{Argument to 'delete' is a constant address (16), which is not memory allocated by 'new' [cplusplus.NewDelete]}}
+              // unix-silenced-warning@-1{{Argument to 'delete' is a constant address (16), which is not memory allocated by 'new' [cplusplus.NewDelete]}}
+}