[clang-tidy] Merge common code between llvmlibc-restrict-system-libc-headers and portability-restrict-system-includes

[PaulkaToast]

Summary

Made llvmlibc::RestrictSystemLibcHeadersCheck a subclass of protability::RestrictSystemIncludesCheck to re-use common code between the two.
This also adds the ability to white list linux development headers.

[aaron.ballman]

LGTM aside from some small nits!

clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt

15

Can you keep this list sorted in alphabetical order?

clang-tools-extra/clang-tidy/portability/RestrictSystemIncludesCheck.h

85

Can you add a newline to the end of the file (as a drive-by)?

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst

32

whitelist -> allow

[PaulkaToast]

Thanks!

clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMLibcModule
   LLVMLibcTidyModule.cpp
   RestrictSystemLibcHeadersCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
+  clangTidyPortabilityModule
   )

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h

 //===--- RestrictSystemLibcHeadersCheck.h - clang-tidy ----------*- 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_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_RESTRICTSYSTEMLIBCHEADERSCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_RESTRICTSYSTEMLIBCHEADERSCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../portability/RestrictSystemIncludesCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_libc {
 
 /// Warns of accidental inclusions of system libc headers that aren't
 /// compiler provided.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.html
-class RestrictSystemLibcHeadersCheck : public ClangTidyCheck {
+class RestrictSystemLibcHeadersCheck
+    : public portability::RestrictSystemIncludesCheck {
 public:
   RestrictSystemLibcHeadersCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : portability::RestrictSystemIncludesCheck(Name, Context, "-*") {}
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
 };
 
 } // namespace llvm_libc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_RESTRICTSYSTEMLIBCHEADERSCHECK_H

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp

 #include "clang/Lex/HeaderSearchOptions.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_libc {
 
 namespace {
 
-class RestrictedIncludesPPCallbacks : public PPCallbacks {
+class RestrictedIncludesPPCallbacks
+    : public portability::RestrictedIncludesPPCallbacks {
 public:
   explicit RestrictedIncludesPPCallbacks(
       RestrictSystemLibcHeadersCheck &Check, const SourceManager &SM,
       const SmallString<128> CompilerIncudeDir)
-      : Check(Check), SM(SM), CompilerIncudeDir(CompilerIncudeDir) {}
+      : portability::RestrictedIncludesPPCallbacks(Check, SM),
+        CompilerIncudeDir(CompilerIncudeDir) {}
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           StringRef SearchPath, StringRef RelativePath,
                           const Module *Imported,
                           SrcMgr::CharacteristicKind FileType) override;
 
 private:
-  RestrictSystemLibcHeadersCheck &Check;
-  const SourceManager &SM;
   const SmallString<128> CompilerIncudeDir;
 };
 
 } // namespace
 
 void RestrictedIncludesPPCallbacks::InclusionDirective(
     SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
     bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
     StringRef SearchPath, StringRef RelativePath, const Module *Imported,
     SrcMgr::CharacteristicKind FileType) {
-  if (SrcMgr::isSystem(FileType)) {
-    // Compiler provided headers are allowed (e.g stddef.h).
-    if (SearchPath == CompilerIncudeDir) return;
-    if (!SM.isInMainFile(HashLoc)) {
-      Check.diag(
-          HashLoc,
-          "system libc header %0 not allowed, transitively included from %1")
-          << FileName << SM.getFilename(HashLoc);
-    } else {
-      Check.diag(HashLoc, "system libc header %0 not allowed") << FileName;
-    }
-  }
+  if (SrcMgr::isSystem(FileType) && SearchPath == CompilerIncudeDir)
+    return;
+  portability::RestrictedIncludesPPCallbacks::InclusionDirective(
+      HashLoc, IncludeTok, FileName, IsAngled, FilenameRange, File, SearchPath,
+      RelativePath, Imported, FileType);
 }
 
 void RestrictSystemLibcHeadersCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   SmallString<128> CompilerIncudeDir =
       StringRef(PP->getHeaderSearchInfo().getHeaderSearchOpts().ResourceDir);
   llvm::sys::path::append(CompilerIncudeDir, "include");
   PP->addPPCallbacks(std::make_unique<RestrictedIncludesPPCallbacks>(
       *this, SM, CompilerIncudeDir));
 }
 
 } // namespace llvm_libc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/portability/RestrictSystemIncludesCheck.h

 
 /// Checks for allowed includes and suggests removal of any others. If no
 /// includes are specified, the check will exit without issuing any warnings.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/portability-restrict-system-includes.html
 class RestrictSystemIncludesCheck : public ClangTidyCheck {
 public:
-  RestrictSystemIncludesCheck(StringRef Name, ClangTidyContext *Context)
+  RestrictSystemIncludesCheck(StringRef Name, ClangTidyContext *Context,
+                              std::string DefaultAllowedIncludes = "*")
       : ClangTidyCheck(Name, Context),
-        AllowedIncludes(Options.get("Includes", "*")),
+        AllowedIncludes(Options.get("Includes", DefaultAllowedIncludes)),
         AllowedIncludesGlobList(AllowedIncludes) {}
 
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool contains(StringRef FileName) {
     return AllowedIncludesGlobList.contains(FileName);
   }
 
 private:
   std::string AllowedIncludes;
   GlobList AllowedIncludesGlobList;
 };
 
+class RestrictedIncludesPPCallbacks : public PPCallbacks {
+public:
+  explicit RestrictedIncludesPPCallbacks(RestrictSystemIncludesCheck &Check,
+                                         const SourceManager &SM)
+      : Check(Check), SM(SM) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override;
+  void EndOfMainFile() override;
+
+private:
+  struct IncludeDirective {
+    IncludeDirective() = default;
+    IncludeDirective(SourceLocation Loc, CharSourceRange Range,
+                     StringRef Filename, StringRef FullPath, bool IsInMainFile)
+        : Loc(Loc), Range(Range), IncludeFile(Filename), IncludePath(FullPath),
+          IsInMainFile(IsInMainFile) {}
+
+    SourceLocation Loc;      // '#' location in the include directive
+    CharSourceRange Range;   // SourceRange for the file name
+    std::string IncludeFile; // Filename as a string
+    std::string IncludePath; // Full file path as a string
+    bool IsInMainFile;       // Whether or not the include is in the main file
+  };
+
+  using FileIncludes = llvm::SmallVector<IncludeDirective, 8>;
+  llvm::SmallDenseMap<FileID, FileIncludes> IncludeDirectives;
+
+  RestrictSystemIncludesCheck &Check;
+  const SourceManager &SM;
+};
+
 } // namespace portability 
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_RESTRICTINCLUDESSCHECK_H
 No newline at end of file

clang-tools-extra/clang-tidy/portability/RestrictSystemIncludesCheck.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Path.h"
 #include <cstring>
 
 namespace clang {
 namespace tidy {
 namespace portability {
 
-class RestrictedIncludesPPCallbacks : public PPCallbacks {
-public:
-  explicit RestrictedIncludesPPCallbacks(RestrictSystemIncludesCheck &Check,
-                                         const SourceManager &SM)
-      : Check(Check), SM(SM) {}
-
-  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
-                          StringRef FileName, bool IsAngled,
-                          CharSourceRange FilenameRange, const FileEntry *File,
-                          StringRef SearchPath, StringRef RelativePath,
-                          const Module *Imported,
-                          SrcMgr::CharacteristicKind FileType) override;
-  void EndOfMainFile() override;
-
-private:
-  struct IncludeDirective {
-    IncludeDirective() = default;
-    IncludeDirective(SourceLocation Loc, CharSourceRange Range,
-                     StringRef Filename, StringRef FullPath, bool IsInMainFile)
-        : Loc(Loc), Range(Range), IncludeFile(Filename), IncludePath(FullPath),
-          IsInMainFile(IsInMainFile) {}
-
-    SourceLocation Loc;      // '#' location in the include directive
-    CharSourceRange Range;   // SourceRange for the file name
-    std::string IncludeFile; // Filename as a string
-    std::string IncludePath; // Full file path as a string
-    bool IsInMainFile;       // Whether or not the include is in the main file
-  };
-
-  using FileIncludes = llvm::SmallVector<IncludeDirective, 8>;
-  llvm::SmallDenseMap<FileID, FileIncludes> IncludeDirectives;
-
-  RestrictSystemIncludesCheck &Check;
-  const SourceManager &SM;
-};
-
 void RestrictedIncludesPPCallbacks::InclusionDirective(
     SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
     bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
     StringRef SearchPath, StringRef RelativePath, const Module *Imported,
     SrcMgr::CharacteristicKind FileType) {
   if (!Check.contains(FileName) && SrcMgr::isSystem(FileType)) {
     SmallString<256> FullPath;
     llvm::sys::path::append(FullPath, SearchPath);

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `hicpp-signed-bitwise <hicpp-signed-bitwise.html>`_,
    `linuxkernel-must-use-errs <linuxkernel-must-use-errs.html>`_,
    `llvm-header-guard <llvm-header-guard.html>`_,
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
-   `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_,
+   `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
    `misc-redundant-expression <misc-redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc-static-assert.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst

.. code-block:: c++
    #include "internal/stdio.h"   // Allowed because it is NOT part of system libc.
 
 
 This check is necessary because accidentally including system libc headers can
 lead to subtle and hard to detect bugs. For example consider a system libc
 whose ``dirent`` struct has slightly different field ordering than llvm-libc.
 While this will compile successfully, this can cause issues during runtime
 because they are ABI incompatible.
+
+Options
+-------
+
+.. option:: Includes
+
+   A string containing a comma separated glob list of allowed include
+   filenames. Similar to the -checks glob list for running clang-tidy itself,
+   the two wildcard characters are `*` and `-`, to include and exclude globs,
+   respectively. The default is `-*`, which disallows all includes.
+
+   This can be used to whitelist known safe includes such as Linux development
+   headers. See :doc:`portability-restrict-system-includes
+   <portability-restrict-system-includes>` for more
+   details.

clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h

-#include <math.h>

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers-transitive.cpp

-// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
-// RUN:   -- -header-filter=.* \
-// RUN:   -- -I %S/Inputs/llvmlibc \
-// RUN:   -isystem %S/Inputs/llvmlibc/system \
-// RUN:   -resource-dir %S/Inputs/llvmlibc/resource
-
-#include "transitive.h"
-// CHECK-MESSAGES: :1:1: warning: system libc header math.h not allowed, transitively included from {{.*}}

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp

 // RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
 // RUN:   -- -- -isystem %S/Inputs/llvmlibc/system \
 // RUN:   -resource-dir %S/Inputs/llvmlibc/resource
 
 #include <stdio.h>
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdio.h not allowed
 #include <stdlib.h>
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdlib.h not allowed
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdlib.h not allowed
 #include "string.h"
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header string.h not allowed
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include string.h not allowed
 #include "stdatomic.h"
 #include <stddef.h>
 // Compiler provided headers should not throw warnings.