[Analyzer][WebKit] NoUncountedMembersChecker

[jkorous]

Updated to reflect feedback from review of the parent patch.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp

105–106

Did you intentionally make a stronger check than the usual SourceManager::isInSystemHeader()? If so i'm curious why - like, what's so special about this checker that requires this? Should we fix all of our other checkers? It must be something something module maps. I don't know much about those.

109

This looks unused.

110

As far as i'm aware, in C++ every RecordDecl is a CXXRecordDecl.

111

What's the motivation for skipping those?

128–133

Can we elaborate a bit on why it's bad? Even something like ... which is error-prone would be great.

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

9

I found members:: fairly unexpected. Maybe simply Class::Member? The surrounding namespace is obvious from the location of the warning and doesn't seem to be worth dumping.

[jkorous]

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp

105–106

No, there was no intention. I'll just use SourceManager::isInSystemHeader(). Thanks for pointing this out!

109

Indeed! Thanks

111

This checker enforces that there are no raw-pointers/references to uncounted types as member variable but since that's actually how WebKit smartpointers are implemented we need to skip those.
I'll add a comment.

128–133

Ok. Shall I add it to the checkers.rst or here?

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

9

Ok.

[jkorous]

• rebased
• addressed some of the comments

[NoQ]

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp

128–133

Here. I'd prefer to avoid this situation where the user reads the warning and is like "ok so what :/".

[NoQ]

Perfect, thank you!

[hokein]

Hi, @jkorous

Thanks for working on this analyzer check.

Just give you some feedback on this check, it seems to trigger the assertion quite often (we noticed that internally there were a lot of crashes).

you can easily reproduce it by running it a LLVM file, e.g. ./bin/clang-tidy -checks="-*,clang-analyzer-webkit*" ../clang-tools-extra/clangd/ClangdLSPServer.cpp

[NoQ]

Looks similar to https://bugs.llvm.org/show_bug.cgi?id=46142. It sounds pretty important to address quickly because it crashes on a fairly large portion of C++ code with default settings.

clang/docs/analyzer/checkers.rst

 .. code-block:: cpp
  struct RefCntblBase {
    void ref() {}
    void deref() {}
  };
 
  struct Derived : RefCntblBase { }; // warn
 
+.. _webkit-WebKitNoUncountedMemberChecker:
+
+webkit.WebKitNoUncountedMemberChecker
+""""""""""""""""""""""""""""""""""""
+Raw pointers and references to uncounted types can't be used as class members. Only ref-counted types are allowed.
+
+.. code-block:: cpp
+ struct RefCntbl {
+   void ref() {}
+   void deref() {}
+ };
+
+ struct Foo {
+   RefCntbl * ptr; // warn
+   RefCntbl & ptr; // warn
+   // ...
+ };
+
 .. _alpha-checkers:
 
 Experimental Checkers
 ---------------------
 
 *These are checkers with known issues or limitations that keep them from being on by default. They are likely to have false positives. Bug reports and especially patches are welcome.*
 
 alpha.clone

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // WebKit checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = WebKit in {
 
 def WebKitRefCntblBaseVirtualDtorChecker : Checker<"WebKitRefCntblBaseVirtualDtor">,
   HelpText<"Check for any ref-countable base class having virtual destructor.">,
   Documentation<HasDocumentation>;
+
+def WebKitNoUncountedMemberChecker : Checker<"WebKitNoUncountedMemberChecker">,
+  HelpText<"Check for no uncounted member variables.">,
+  Documentation<HasDocumentation>;
 } // end webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   UninitializedObject/UninitializedObjectChecker.cpp
   UninitializedObject/UninitializedPointee.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
   VforkChecker.cpp
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp
+  WebKit/NoUncountedMembersChecker.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
   )

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp

+//=======- NoUncountedMembersChecker.cpp -------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTUtils.h"
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class NoUncountedMemberChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+private:
+  BugType Bug;
+  mutable BugReporter *BR;
+
+public:
+  NoUncountedMemberChecker()
+      : Bug(this,
+            "Member variable is a raw-poiner/reference to reference-countable "
+            "type",
+            "WebKit coding guidelines") {}
+
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
+                    BugReporter &BRArg) const {
+    BR = &BRArg;
+
+    // The calls to checkAST* from AnalysisConsumer don't
+    // visit template instantiations or lambda classes. We
+    // want to visit those, so we make our own RecursiveASTVisitor.
+    struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
+      const NoUncountedMemberChecker *Checker;
+      explicit LocalVisitor(const NoUncountedMemberChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitRecordDecl(const RecordDecl *RD) {
+        Checker->visitRecordDecl(RD);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitRecordDecl(const RecordDecl *RD) const {
+    if (shouldSkipDecl(RD))
+      return;
+
+    for (auto Member : RD->fields()) {
+      const Type *MemberType = Member->getType().getTypePtrOrNull();
+      if (!MemberType)
+        continue;
+
+      if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
+        if (isRefCountable(MemberCXXRD))
+          reportBug(Member, MemberType, MemberCXXRD, RD);
+      }
+    }
+  }
+
+  bool shouldSkipDecl(const RecordDecl *RD) const {
+    if (!RD->isThisDeclarationADefinition())
+      return true;
+
+    if (RD->isImplicit())
+      return true;
+
+    if (RD->isLambda())
+      return true;
+
+    // If the construct doesn't have a source file, then it's not something
+    // we want to diagnose.
+    const auto RDLocation = RD->getLocation();
+    if (!RDLocation.isValid())
+      return true;
+
+    const auto Kind = RD->getTagKind();
+    // FIMXE: Should we check union members too?
+    if (Kind != TTK_Struct && Kind != TTK_Class)
+      return true;
+
+    // Ignore CXXRecords that come from system headers.
+    if (BR->getSourceManager().getFileCharacteristic(RDLocation) !=
+        SrcMgr::C_User)
+      return true;
+
+    const auto ClassName = safeGetName(RD);
+    if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+      if (isRefCounted(CXXRD))
+        return true;
+    }
+
+    return false;
+  }
+
+  void reportBug(const FieldDecl *Member, const Type *MemberType,
+                 const CXXRecordDecl *MemberCXXRD,
+                 const RecordDecl *ClassCXXRD) const {
+    assert(Member);
+    assert(MemberType);
+    assert(MemberCXXRD);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    Os << "Member variable ";
+    printQuotedQualifiedName(Os, Member);
+
+    Os << " is a "
+       << (isa<PointerType>(MemberType) ? "raw pointer" : "reference")
+       << " to ref-countable type ";
+    printQuotedQualifiedName(Os, MemberCXXRD);
+
+    PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(),
+                                 BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(Member->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerWebKitNoUncountedMemberChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NoUncountedMemberChecker>();
+}
+
+bool ento::shouldRegisterWebKitNoUncountedMemberChecker(const LangOptions &LO) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitNoUncountedMemberChecker -verify %s
+
+#include "mock-types.h"
+
+namespace members {
+  struct Foo {
+  private:
+    RefCountable* a = nullptr;
+// expected-warning@-1{{Member variable 'members::Foo::a' is a raw pointer to ref-countable type 'RefCountable'}}
+
+  protected:
+    RefPtr<RefCountable> b;
+
+  public:
+    RefCountable silenceWarningAboutInit;
+    RefCountable& c = silenceWarningAboutInit;
+// expected-warning@-1{{Member variable 'members::Foo::c' is a reference to ref-countable type 'RefCountable'}}
+    Ref<RefCountable> d;
+  };
+
+  template<class T>
+  struct FooTmpl {
+    T* a;
+// expected-warning@-1{{Member variable 'members::FooTmpl<RefCountable>::a' is a raw pointer to ref-countable type 'RefCountable'}}
+  };
+
+  void forceTmplToInstantiate(FooTmpl<RefCountable>) {}
+}
+
+namespace ignore_unions {
+  union Foo {
+    RefCountable* a;
+    RefPtr<RefCountable> b;
+    Ref<RefCountable> c;
+  };
+
+  template<class T>
+  union RefPtr {
+    T* a;
+  };
+
+  void forceTmplToInstantiate(RefPtr<RefCountable>) {}
+}