[Analyzer][WebKit] RefCntblBaseVirtualDtorChecker & shared utils

[jkorous]

Summary

I'm starting to put up patches for WebKit checkers in analyzer. I have the first batch of 3 checkers pretty much ready and would like to get some feedback before I proceed with the rest.

I also have a bunch of questions.

• ASTMatchers
  • I was considering rewriting the visitors to matchers but since I haven't really used I don't have intuition for them. From other uses it seems to me I won't be able to express all the logic currently implemented in shouldSkipFoo methods (including next patches too). Would having only part of that logic expressed as matcher still be a win?

• documentation
  • What does Documentation<HasDocumentation> in Checkers.tddo?
  • Also, the checkers for WebKit are designed as a system of rules and although there are no implementation dependencies between individual checkers they make sense only as a whole. What would be a good place for description of the rules? Just a README in clang/lib/StaticAnalyzer/Checkers/WebKit or somewhere in clang/docs/analyzer?

• tests organization
  • Seems like the convention is one file per checker. Correct?

[jkorous]

Fix typo in doc comment

[NoQ]

ASTMatchers

They're very flexible. You can express arbitrary parts of the logic with them, you can combine and re-use the combinations by putting them into variables or producing via factory functions. On top of that, it's ridiculously easy to define your own completely custom matchers if you're interested in, say, making queries about a specific property of the node that nobody queried before with matchers; such custom matchers may be either made public or remain local to your checker.

Your code looks very matcher-ish. I wouldn't be surprised if you can express it with a few matchers with minimal customizations.

The "skip" part is done via unless() matcher. Base class traversal is done by isDerivedFrom() matchers that accept arbitrary sub-matchers for the base class. Etc.

documentation

https://clang-analyzer.llvm.org/available_checks.html is where it should ultimately appear. In the monorepo it's in clang/www/analyzer in plain html. I think Documentation<HasDocumentation> has no formal meaning for now, just a markup for easier figuring out if we need one; @Szelethus am i right?

Seems like the convention is one file per checker. Correct?

Usually yes, but very situational. Say, if the checker checks global operator new, you clearly can't have all tests in one file. Some checkers work for different languages so again more files. Some tests test interactions between different checkers. Some tests are just huge so people decided to put them in different files.

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

1630

"Alpha" is an indication that you want to do more work on these checkers before enabling them by default. What work do you foresee?

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h

28–29

A code or AST dump example would have been super helpful. I don't understand what "Origin" means. Is this an lvalue that the expression loads from?

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

24

Why not directly check::ASTDecl<CXXRecordDecl>? (I don't remember what exactly does it do).

26–27

These days we don't do lazy initialization anymore, just BugType Bug{"Description", "Category"}.

138

Unlike clang frontend, we usually encourage our warnings and notes to sound like full sentences; there's no need to shorten or abbreviate things in order to fit into 80 characters of a terminal or something because we're by design a GUI tool, so understandability comes first.

139

WebKit isn't a category of bugs, it's a browser engine :) I suggest something like "WebKit coding guidelines" or something along those lines.

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp

11

Do you ultimately want to have notes as well? Like, point the user to RefCntblBase or something? 'Cause we do have the ability to emit path-insensitive notes but they're only implemented in scan-build at this point.

[martong]

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

23

This checker does not seem to benefit anything from path sensitivity. Would it be more appropriate as a clang-tidy (with ASTMatchers) checker?

[dcoughlin]

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

23

We have many AST-based checks in the Clang Static Analyzer (some of which use ASTMatchers). I'd like to have this in the static analyzer so that static analyzer users can benefit from it.

[NoQ]

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

23

Yeah, for now the answer to such questions is "whatever is the tool that the author uses", because the engine isn't the only thing that's different. Ideally when i finish my work on providing different output modes to clang-tidy and we also somehow allow transparently running clang-tidy on top of an existing static analyzer integration, we may move all of our AST-based checks to clang-tidy and finally solve this dilemma :)

[Szelethus]

I'll attend to your patch when I find the time!

In D77177#1953895, @NoQ wrote:

documentation

https://clang-analyzer.llvm.org/available_checks.html is where it should ultimately appear. In the monorepo it's in clang/www/analyzer in plain html.

I disagree, I think we should focus on the official documentation (https://clang.llvm.org/docs/analyzer/checkers.html), and try to keep the plain html coding to the minimum.

I think Documentation<HasDocumentation> has no formal meaning for now, just a markup for easier figuring out if we need one; @Szelethus am i right?

This was actually added by Aaron in D55792.

The static analysis checkers are mostly listed on two pages, one for alpha checks and one for released checks (there are also a considerable number of undocumented analyzer checkers). This patch adds anchors to all of the documented checks so that you can directly link to a check by a stable name. This is useful because the SARIF file format has a field for specifying a URI to documentation for a rule and some viewers, like CodeSonar, make use of this information.

This patch exposes those anchors through the SARIF exporter, which requires an automated way to determine the check URIs. Checkers.td now requires each checker to be annotated with an extensible list of documentation kinds (listed in CheckerBase.td) that map to physical URI paths on the static analyzer web page if the check has documentation (mapping is done by the table generator), which is exposed via the CHECKER macro. When exporting to SARIF, the checker name itself is used as the anchor target on the page (and I verified that checker names, including dots and dashes, will be valid anchors) and the corresponding edits were made to each HTML file to add the anchors.

The generated links still point to clang-analyzer.llvm.org instead of the official clang documentation, we should probably change that soon.

[jkorous]

Sorry - I should have made this clear earlier so nobody waste their time reviewing current version - I'm rewriting this with ASTMatchers.

[jkorous]

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

1630

I intended to keep all the checks initially as alpha but maybe it's unnecessary.

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h

28–29

I don't know what the best word would be here but the "origin" or "source" is an AST node which ultimately provides the memory address used in Expr E.

Some examples where we successfully find the origin:

void foo(RefCountable* ptr);

foo(nullptr); // origin of ptr is nullptr

RefCountable* local_ptr;
foo( *&local_ptr ); /// origin of ptr is local_ptr

void bar(RefPtr<RefCountable> refcntd_ptr_param) {
   foo( &makeRef(refcntd_ptr_param.get()).get() ); /// origin of ptr is refcntd_ptr_param
}

Some examples where we don't find the origin:

void foo(RefCountable* ptr);
RefCountable* provide_ref_cntbl();
RefCountable* somehow_combine_ptrs(RefCountable*, RefCountable*);

foo(provide_ref_cntbl()); // tryToFindPtrOrigin would return the CallExpr

foo( makeRefPtr(provide_ref_cntbl()).get() ); // tryToFindPtrOrigin would return the CallExpr to provide_ref_cntbl()

foo( makeRefPtr( somehow_combine_ptrs(provide_ref_cntbl(), provide_ref_cntbl()) ); // tryToFindPtrOrigin would return the CallExpr to somehow_combine_ptrs()

I think that ultimately the right place would be the documentation and I'd add a link here. Also, I should add tests and that might help with understanding too. WDYT?

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

138

I'll fix that.
BTW why is the dot at the end stripped away? I was a bit surprised but then I found out that it's intentional :)

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp

11

I wasn't thinking about creating notes. I guess with good enough documentation that'd explain the concern we might be fine?

[jkorous]

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

24

Oh! You're right! I just copied this from some other checker.

With check::ASTDecl<CXXRecordDecl> I can nuke the whole ugly LocalVistitor!

But... Do you happen to know how can I visit template instantiations? In the ASTVisitor it's easy - I just override shouldVisitTemplateInstantiations().

Since I see this node in the AST in my tests:

ClassTemplateSpecializationDecl 0x7fa6010a3538 <line:25:1, line:26:32> col:8 struct DerivedClassTmpl3 definition

I tried:

class RefCntblBaseVirtualDtorChecker
    : public Checker<
      check::ASTDecl<CXXRecordDecl>,
      check::ASTDecl<ClassTemplateSpecializationDecl>
    > {
//...

  void checkClassTemplateSpecializationDecl(
// ...
  ) const {
    checkASTDecl(
      dyn_cast<ClassTemplateSpecializationDecl>(CTSD),
      MGR, BRArg);
  }

but seems like that's not it. I should probably think more about what's the relation between instantiations and specializations here.

I'd prefer not to copy the logic from RecursiveASTVisitor.h if at all possible.

[jkorous]

Not all comments have been addressed yet but we can continue the conversation.

[jkorous]

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

24

I think I kind of understand how this works now.

IIUC for check::ASTDecl-based checkers the visitor that invokes them is AnalysisConsumer. The thing that I don't see is a hypothetical switch that I could flip from within checker's implementation that would cause the AnalysisConsumer to visit template instantiations.

I tried hardcoding this in the AnalysisConsumer implementation by adding the following line to AnalysisConsumer.cpp and it worked like a charm.

bool shouldVisitTemplateInstantiations() const { return true; }

Now, I could imagine that for visitor which is the foundation for all AST-based checkers you might want to do this but 1. my context is infinitesimal 2. it's a positively scary thing to do now that checkers might rely on current behavior.

So, the approach with check::ASTDecl<TranslationUnitDecl> and defining my own visitor where I can locally tweak its behavior as I please can be seen as a workaround.

Since I actually copied the approach from PaddingChecker it seems that occasionally people run into this.

[jkorous]

Addressed the comments

[jkorous]

Ping

[NoQ]

Thanks for the investigations! My only remaining concern is about tryToFindPtrOrigin(). I suspect it's not used as part of this patch; maybe we can land it in a later patch in which i can see how it's used?

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h

28–29

I don't know what the best word would be here

Mm, can you describe it with multiple words? Because i'm completely confused at the moment. Like, i get the rough idea but i don't know how to apply it outside of the examples that you've already provided. For instance, both expressions ****p and a->b->c->d->e mention ~5 completely unrelated memory addresses; which one of those do you intend to grab? It would generally help a lot if you use words like "lvalue", "rvalue", "implicit lvalue-to-rvalue cast" (i believe that this cast kind most likely requires special treatment in your implementation).

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

24

Aha, yup, ok. We have too few checkers for checkASTDecl<CXXRecordDecl> to figure out whether we want shouldVisitTemplateInstantiations() on or off by default. I think most checkers would ultimately want it on, but on the other hand it is challenging to provide a good diagnostic when you're describing a problem in an instantiation but all your source locations point to the original template. So it'd be an unobvious way to shoot yourself in the foot if we had it enabled by default and some checkers didn't take it into account. You printed out the type with template parameters and this seems more or less sufficient (though a note at the point of instantiation could also have helped). So let's keep it as is!

[jkorous]

Addressed comments:

• removed utils not necessary for this patch
• added documentation
• clang-format

[jkorous]

I added documentation for the checker to clang/docs/analyzer/checkers.rst since clang/www/analyzer/available_checks.html seems to be out of date. Shall I still add it there too?

[NoQ]

Thank you, let's land this!

[NoQ]

In D77177#2035714, @jkorous wrote:

I added documentation for the checker to clang/docs/analyzer/checkers.rst since clang/www/analyzer/available_checks.html seems to be out of date. Shall I still add it there too?

Yeah, we'll have to clean it up eventually, don't worry for now.

clang/docs/analyzer/checkers.rst

1383

It might be useful to precede the list with an explanation of what that list is of. Eg., "The checker reacts on the following WebKit classes and methods:" or "Hereinafter the following terminology is used for describing the available checkers:" or something along those lines.

[thakis]

This breaks the build everywhere (e.g. http://45.33.8.238/linux/18283/step_4.txt) and has been in for over an hour. Please watch bots after landing. (I think changes that are created more recently automatically get presubmit compile testing on phab.) Reverted in 1108f5c737dbdab0277874a7e5b237491839c43a for now.

[thakis]

About the patch itself: Project-specific checks like this usually go in clang-tidy, not in the static analyzer (which ships with the compiler binary).

[NoQ]

Reverted in 1108f5c737dbdab0277874a7e5b237491839c43a for now.

Uh-oh. Thank you for reverting!

Project-specific checks like this usually go in clang-tidy, not in the static analyzer (which ships with the compiler binary).

There are a lot more considerations to be taken into account when choosing where to put the check. The static analyzer already contains a lot of project-specific and framework-specific checks which will never be implemented in clang-tidy because they're path-sensitive. Also these tools are not a drop-in replacement of each other: you cannot use clang-tidy in all the places where you can use the static analyzer (the opposite is partially true due to libStaticAnalyzer integration into clang-tidy but UI degrades dramatically when such integration is used) therefore many authors simply do not have a choice where to put the check (i'm slowly working on improving this situation so that most path-insensitive checks could indeed be migrated into clang-tidy but we're not there yet). So as of today we have no choice but to let our contributors decide freely where they want their checks to be.

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

1632

While we're at it, maybe remove "WebKit" from the checker name, given that it duplicates the package name?

[jkorous]

In D77177#2049805, @thakis wrote:

This breaks the build everywhere (e.g. http://45.33.8.238/linux/18283/step_4.txt) and has been in for over an hour. Please watch bots after landing. (I think changes that are created more recently automatically get presubmit compile testing on phab.) Reverted in 1108f5c737dbdab0277874a7e5b237491839c43a for now.

Sorry about the mess! I shouldn't have landed the patch just before I stepped out for 2 hrs...

clang/docs/analyzer/checkers.rst

void checkLeak08(int tag) {
    zx_handle_t sa, sb;
    zx_channel_create(0, &sa, &sb);
    if (tag)
      zx_handle_close(sa);
    use(sb); // Warn: Potential leak of handle
    zx_handle_close(sb);
  }
 
+WebKit
+^^^^^^
+
+WebKit is an open-source web browser engine available for macOS, iOS and Linux.
+This section describes checkers that can find issues in WebKit codebase.
+
+Most of the checkers focus on memory management for which WebKit uses custom implementation of reference counted smartpointers.
+Checker are formulated in terms related to ref-counting:
+* *Ref-counted type* is either ``Ref<T>`` or ``RefPtr<T>``.
+* *Ref-countable type* is any type that implements ``ref()`` and ``deref()`` methods as ``RefPtr<>`` is a template (i. e. relies on duck typing).
+* *Uncounted type* is ref-countable but not ref-counted type.
+
+.. _webkit-WebKitRefCntblBaseVirtualDtor:
+
+webkit.WebKitRefCntblBaseVirtualDtor
+""""""""""""""""""""""""""""""""""""
+All uncounted types used as base classes must have a virtual destructor.
+
+Ref-counted types hold their ref-countable data by a raw pointer and allow implicit upcasting from ref-counted pointer to derived type to ref-counted pointer to base type. This might lead to an object of (dynamic) derived type being deleted via pointer to the base class type which C++ standard defines as UB in case the base class doesn't have virtual destructor ``[expr.delete]``.
+
+.. code-block:: cpp
+ struct RefCntblBase {
+   void ref() {}
+   void deref() {}
+ };
+
+ struct Derived : RefCntblBase { }; // 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.*
 

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

 
 def CloneDetectionAlpha : Package<"clone">, ParentPackage<Alpha>;
 
 def NonDeterminismAlpha : Package<"nondeterminism">, ParentPackage<Alpha>;
 
 def Fuchsia : Package<"fuchsia">;
 def FuchsiaAlpha : Package<"fuchsia">, ParentPackage<Alpha>;
 
+def WebKit : Package<"webkit">;
+def WebKitAlpha : Package<"webkit">, ParentPackage<Alpha>;
+
 //===----------------------------------------------------------------------===//
 // Core Checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Core in {
 
 def CallAndMessageModeling : Checker<"CallAndMessageModeling">,
   HelpText<"Responsible for essential modeling and assumptions after a "

 
 def FuchsiaLockChecker : Checker<"Lock">,
   HelpText<"Check for the correct usage of locking APIs.">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasDocumentation>;
 
 } // end fuchsia
 
+//===----------------------------------------------------------------------===//
+// WebKit checkers.
+//===----------------------------------------------------------------------===//
+
+let ParentPackage = WebKit in {
+
+def WebKitRefCntblBaseVirtualDtorChecker : Checker<"WebKitRefCntblBaseVirtualDtor">,
+  HelpText<"Check for any ref-countable base class having virtual destructor.">,
+  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/PtrTypesSemantics.cpp
+  WebKit/RefCntblBaseVirtualDtorChecker.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
   )

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h

+//=======- ASTUtis.h ---------------------------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYZER_WEBKIT_ASTUTILS_H
+#define LLVM_CLANG_ANALYZER_WEBKIT_ASTUTILS_H
+
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/Support/Casting.h"
+
+#include <string>
+#include <utility>
+
+namespace clang {
+class CXXRecordDecl;
+class CXXBaseSpecifier;
+class FunctionDecl;
+class CXXMethodDecl;
+class Expr;
+
+/// If passed expression is of type uncounted pointer/reference we try to find
+/// the origin of this pointer. Example: Origin can be a local variable, nullptr
+/// constant or this-pointer.
+///
+/// Certain subexpression nodes represent transformations that don't affect
+/// where the memory address originates from. We try to traverse such
+/// subexpressions to get to the relevant child nodes. Whenever we encounter a
+/// subexpression that either can't be ignored, we don't model its semantics or
+/// that has multiple children we stop.
+///
+/// \p E is an expression of uncounted pointer/reference type.
+/// If \p StopAtFirstRefCountedObj is true and we encounter a subexpression that
+/// represents ref-counted object during the traversal we return relevant
+/// sub-expression and true.
+///
+/// \returns subexpression that we traversed to and if \p
+/// StopAtFirstRefCountedObj is true we also return whether we stopped early.
+std::pair<const clang::Expr *, bool>
+tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj);
+
+/// For \p E referring to a ref-countable/-counted pointer/reference we return
+/// whether it's a safe call argument. Examples: function parameter or
+/// this-pointer. The logic relies on the set of recursive rules we enforce for
+/// WebKit codebase.
+///
+/// \returns Whether \p E is a safe call arugment.
+bool isASafeCallArg(const clang::Expr *E);
+
+/// \returns name of AST node or empty string.
+template <typename T> std::string safeGetName(const T *ASTNode) {
+  const auto *const ND = llvm::dyn_cast_or_null<clang::NamedDecl>(ASTNode);
+  if (!ND)
+    return "";
+
+  // In case F is for example "operator|" the getName() method below would
+  // assert.
+  if (!ND->getDeclName().isIdentifier())
+    return "";
+
+  return ND->getName().str();
+}
+
+} // namespace clang
+
+#endif

clang/lib/StaticAnalyzer/Checkers/WebKit/DiagOutputUtils.h

+//=======- DiagOutputUtils.h -------------------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYZER_WEBKIT_DIAGPRINTUTILS_H
+#define LLVM_CLANG_ANALYZER_WEBKIT_DIAGPRINTUTILS_H
+
+#include "clang/AST/Decl.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+
+template <typename NamedDeclDerivedT>
+void printQuotedQualifiedName(llvm::raw_ostream &Os,
+                              const NamedDeclDerivedT &D) {
+  Os << "'";
+  D->getNameForDiagnostic(Os, D->getASTContext().getPrintingPolicy(),
+                          /*Qualified=*/true);
+  Os << "'";
+}
+
+} // namespace clang
+
+#endif

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h

+//=======- PtrTypesSemantics.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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
+#define LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
+
+namespace clang {
+class CXXBaseSpecifier;
+class CXXMethodDecl;
+class CXXRecordDecl;
+class Expr;
+class FunctionDecl;
+class Type;
+
+// Ref-countability of a type is implicitly defined by Ref<T> and RefPtr<T>
+// implementation. It can be modeled as: type T having public methods ref() and
+// deref()
+
+// In WebKit there are two ref-counted templated smart pointers: RefPtr<T> and
+// Ref<T>.
+
+/// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if
+/// not.
+const clang::CXXRecordDecl *isRefCountable(const clang::CXXBaseSpecifier *Base);
+
+/// \returns true if \p Class is ref-countable, false if not.
+/// Asserts that \p Class IS a definition.
+bool isRefCountable(const clang::CXXRecordDecl *Class);
+
+/// \returns true if \p Class is ref-counted, false if not.
+bool isRefCounted(const clang::CXXRecordDecl *Class);
+
+/// \returns true if \p Class is ref-countable AND not ref-counted, false if
+/// not. Asserts that \p Class IS a definition.
+bool isUncounted(const clang::CXXRecordDecl *Class);
+
+/// \returns true if \p T is either a raw pointer or reference to an uncounted
+/// class, false if not.
+bool isUncountedPtr(const clang::Type *T);
+
+/// \returns true if \p F creates ref-countable object from uncounted parameter,
+/// false if not.
+bool isCtorOfRefCounted(const clang::FunctionDecl *F);
+
+/// \returns true if \p M is getter of a ref-counted class, false if not.
+bool isGetterOfRefCounted(const clang::CXXMethodDecl *Method);
+
+/// \returns true if \p F is a conversion between ref-countable or ref-counted
+/// pointer types.
+bool isPtrConversion(const FunctionDecl *F);
+
+} // namespace clang
+
+#endif

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

+//=======- PtrTypesSemantics.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 "PtrTypesSemantics.h"
+#include "ASTUtils.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
+
+using llvm::Optional;
+using namespace clang;
+
+namespace {
+
+bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
+  assert(R);
+
+  bool hasRef = false;
+  bool hasDeref = false;
+  for (const CXXMethodDecl *MD : R->methods()) {
+    const auto MethodName = safeGetName(MD);
+
+    if (MethodName == "ref" && MD->getAccess() == AS_public) {
+      if (hasDeref)
+        return true;
+      hasRef = true;
+    } else if (MethodName == "deref" && MD->getAccess() == AS_public) {
+      if (hasRef)
+        return true;
+      hasDeref = true;
+    }
+  }
+  return false;
+}
+
+} // namespace
+
+namespace clang {
+
+const CXXRecordDecl *isRefCountable(const CXXBaseSpecifier *Base) {
+  assert(Base);
+
+  const Type *T = Base->getType().getTypePtrOrNull();
+  if (!T)
+    return nullptr;
+
+  const CXXRecordDecl *R = T->getAsCXXRecordDecl();
+  if (!R)
+    return nullptr;
+
+  return hasPublicRefAndDeref(R) ? R : nullptr;
+};
+
+bool isRefCountable(const CXXRecordDecl *R) {
+  assert(R);
+
+  R = R->getDefinition();
+  assert(R);
+
+  if (hasPublicRefAndDeref(R))
+    return true;
+
+  CXXBasePaths Paths;
+  Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
+
+  const auto isRefCountableBase = [](const CXXBaseSpecifier *Base,
+                                     CXXBasePath &) {
+    return clang::isRefCountable(Base);
+  };
+
+  return R->lookupInBases(isRefCountableBase, Paths,
+                          /*LookupInDependent =*/true);
+}
+
+bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
+  assert(F);
+  const auto &FunctionName = safeGetName(F);
+
+  return FunctionName == "Ref" || FunctionName == "makeRef"
+
+         || FunctionName == "RefPtr" || FunctionName == "makeRefPtr"
+
+         || FunctionName == "UniqueRef" || FunctionName == "makeUniqueRef" ||
+         FunctionName == "makeUniqueRefWithoutFastMallocCheck"
+
+         || FunctionName == "String" || FunctionName == "AtomString" ||
+         FunctionName == "UniqueString"
+         // FIXME: Implement as attribute.
+         || FunctionName == "Identifier";
+}
+
+bool isUncounted(const CXXRecordDecl *Class) {
+  // Keep isRefCounted first as it's cheaper.
+  return !isRefCounted(Class) && isRefCountable(Class);
+}
+
+bool isUncountedPtr(const Type *T) {
+  assert(T);
+
+  if (T->isPointerType() || T->isReferenceType()) {
+    if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
+      return isUncounted(CXXRD);
+    }
+  }
+  return false;
+}
+
+bool isGetterOfRefCounted(const CXXMethodDecl *M) {
+  assert(M);
+
+  if (auto *calleeMethodDecl = dyn_cast<CXXMethodDecl>(M)) {
+    const CXXRecordDecl *calleeMethodsClass = M->getParent();
+    auto className = safeGetName(calleeMethodsClass);
+    auto methodName = safeGetName(M);
+
+    if (((className == "Ref" || className == "RefPtr") &&
+         methodName == "get") ||
+        ((className == "String" || className == "AtomString" ||
+          className == "AtomStringImpl" || className == "UniqueString" ||
+          className == "UniqueStringImpl" || className == "Identifier") &&
+         methodName == "impl"))
+      return true;
+
+    // Ref<T> -> T conversion
+    // FIXME: Currently allowing any Ref<T> -> whatever cast.
+    if (className == "Ref" || className == "RefPtr") {
+      if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) {
+        if (auto *targetConversionType =
+                maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) {
+          if (isUncountedPtr(targetConversionType)) {
+            return true;
+          }
+        }
+      }
+    }
+  }
+  return false;
+}
+
+bool isRefCounted(const CXXRecordDecl *R) {
+  assert(R);
+  if (auto *TmplR = R->getTemplateInstantiationPattern()) {
+    // FIXME: String/AtomString/UniqueString
+    const auto &ClassName = safeGetName(TmplR);
+    return ClassName == "RefPtr" || ClassName == "Ref";
+  }
+  return false;
+}
+
+bool isPtrConversion(const FunctionDecl *F) {
+  assert(F);
+  if (isCtorOfRefCounted(F))
+    return true;
+
+  // FIXME: check # of params == 1
+  const auto FunctionName = safeGetName(F);
+  if (FunctionName == "getPtr" || FunctionName == "WeakPtr" ||
+      FunctionName == "makeWeakPtr"
+
+      || FunctionName == "downcast" || FunctionName == "bitwise_cast")
+    return true;
+
+  return false;
+}
+
+} // namespace clang

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

+//=======- RefCntblBaseVirtualDtor.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 "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.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"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class RefCntblBaseVirtualDtorChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+private:
+  BugType Bug;
+  mutable BugReporter *BR;
+
+public:
+  RefCntblBaseVirtualDtorChecker()
+      : Bug(this,
+            "Reference-countable base class doesn't have virtual destructor",
+            "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 RefCntblBaseVirtualDtorChecker *Checker;
+      explicit LocalVisitor(const RefCntblBaseVirtualDtorChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitCXXRecordDecl(const CXXRecordDecl *RD) {
+        Checker->visitCXXRecordDecl(RD);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitCXXRecordDecl(const CXXRecordDecl *RD) const {
+    if (shouldSkipDecl(RD))
+      return;
+
+    CXXBasePaths Paths;
+    Paths.setOrigin(RD);
+
+    const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
+    const CXXRecordDecl *ProblematicBaseClass = nullptr;
+
+    const auto IsPublicBaseRefCntblWOVirtualDtor =
+        [RD, &ProblematicBaseSpecifier,
+         &ProblematicBaseClass](const CXXBaseSpecifier *Base, CXXBasePath &) {
+          const auto AccSpec = Base->getAccessSpecifier();
+          if (AccSpec == AS_protected || AccSpec == AS_private ||
+              (AccSpec == AS_none && RD->isClass()))
+            return false;
+
+          llvm::Optional<const clang::CXXRecordDecl *> MaybeRefCntblBaseRD =
+              isRefCountable(Base);
+          if (!MaybeRefCntblBaseRD.hasValue())
+            return false;
+
+          const CXXRecordDecl *RefCntblBaseRD = MaybeRefCntblBaseRD.getValue();
+          if (!RefCntblBaseRD)
+            return false;
+
+          const auto *Dtor = RefCntblBaseRD->getDestructor();
+          if (!Dtor || !Dtor->isVirtual()) {
+            ProblematicBaseSpecifier = Base;
+            ProblematicBaseClass = RefCntblBaseRD;
+            return true;
+          }
+
+          return false;
+        };
+
+    if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
+                          /*LookupInDependent =*/true)) {
+      reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+    }
+  }
+
+  bool shouldSkipDecl(const CXXRecordDecl *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();
+    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;
+
+    return false;
+  }
+
+  void reportBug(const CXXRecordDecl *DerivedClass,
+                 const CXXBaseSpecifier *BaseSpec,
+                 const CXXRecordDecl *ProblematicBaseClass) const {
+    assert(DerivedClass);
+    assert(BaseSpec);
+    assert(ProblematicBaseClass);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    Os << (ProblematicBaseClass->isClass() ? "Class" : "Struct") << " ";
+    printQuotedQualifiedName(Os, ProblematicBaseClass);
+
+    Os << " is used as a base of "
+       << (DerivedClass->isClass() ? "class" : "struct") << " ";
+    printQuotedQualifiedName(Os, DerivedClass);
+
+    Os << " but doesn't have virtual destructor";
+
+    PathDiagnosticLocation BSLoc(BaseSpec->getSourceRange().getBegin(),
+                                 BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(BaseSpec->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerWebKitRefCntblBaseVirtualDtorChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<RefCntblBaseVirtualDtorChecker>();
+}
+
+bool ento::shouldRegisterWebKitRefCntblBaseVirtualDtorChecker(
+    const LangOptions &LO) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/mock-types.h

+#ifndef mock_types_1103988513531
+#define mock_types_1103988513531
+
+template <typename T> struct Ref {
+  T t;
+
+  Ref() : t{} {};
+  Ref(T *) {}
+  T *get() { return nullptr; }
+  operator const T &() const { return t; }
+  operator T &() { return t; }
+};
+
+template <typename T> struct RefPtr {
+  T *t;
+
+  RefPtr() : t(new T) {}
+  RefPtr(T *t) : t(t) {}
+  T *get() { return t; }
+  T *operator->() { return t; }
+  T &operator*() { return *t; }
+  RefPtr &operator=(T *) { return *this; }
+};
+
+template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {
+  return false;
+}
+
+template <typename T> bool operator==(const RefPtr<T> &, T *) { return false; }
+
+template <typename T> bool operator==(const RefPtr<T> &, T &) { return false; }
+
+template <typename T> bool operator!=(const RefPtr<T> &, const RefPtr<T> &) {
+  return false;
+}
+
+template <typename T> bool operator!=(const RefPtr<T> &, T *) { return false; }
+
+template <typename T> bool operator!=(const RefPtr<T> &, T &) { return false; }
+
+struct RefCountable {
+  void ref() {}
+  void deref() {}
+};
+
+template <typename T> T *downcast(T *t) { return t; }
+
+#endif

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitRefCntblBaseVirtualDtor -verify %s
+
+struct RefCntblBase {
+  void ref() {}
+  void deref() {}
+};
+
+template<class T>
+struct DerivedClassTmpl1 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl1<RefCntblBase>' but doesn't have virtual destructor}}
+
+DerivedClassTmpl1<RefCntblBase> a;
+
+
+
+template<class T>
+struct DerivedClassTmpl2 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl2<RefCntblBase>' but doesn't have virtual destructor}}
+
+template<class T> int foo(T) { DerivedClassTmpl2<T> f; return 42; }
+int b = foo(RefCntblBase{});
+
+
+
+template<class T>
+struct DerivedClassTmpl3 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl3<RefCntblBase>' but doesn't have virtual destructor}}
+
+typedef DerivedClassTmpl3<RefCntblBase> Foo;
+Foo c;

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitRefCntblBaseVirtualDtor -verify %s
+
+struct RefCntblBase {
+  void ref() {}
+  void deref() {}
+};
+
+struct Derived : RefCntblBase { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'Derived' but doesn't have virtual destructor}}
+
+struct DerivedWithVirtualDtor : RefCntblBase {
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedWithVirtualDtor' but doesn't have virtual destructor}}
+  virtual ~DerivedWithVirtualDtor() {}
+};
+
+
+
+template<class T>
+struct DerivedClassTmpl : T { };
+typedef DerivedClassTmpl<RefCntblBase> Foo;
+
+
+
+struct RandomBase {};
+struct RandomDerivedClass : RandomBase { };
+
+
+
+struct FakeRefCntblBase1 {
+  private:
+  void ref() {}
+  void deref() {}
+};
+struct Quiet1 : FakeRefCntblBase1 {};
+
+struct FakeRefCntblBase2 {
+  protected:
+  void ref() {}
+  void deref() {}
+};
+struct Quiet2 : FakeRefCntblBase2 {};
+
+class FakeRefCntblBase3 {
+  void ref() {}
+  void deref() {}
+};
+struct Quiet3 : FakeRefCntblBase3 {};
+struct Quiet4 : private RefCntblBase {};
+class Quiet5 : RefCntblBase {};
+
+void foo () {
+  Derived d;
+}