PaulkaToast updated this revision to Diff 248851.
PaulkaToast added a comment.
Thanks for the suggestions, the general check sounds like a great idea. I can
see the use case for this as it can be used by anyone. I took the time to port
out fuchsia's check and flesh out the user facing documentation. Here is the
patch for that D75786 <https://reviews.llvm.org/D75786>.
Keeping that in mind, I believe using the general check with a list of headers
in llvm-libc's case is a bad idea due the following edge cases:
1. The compiler stops providing an include
------------------------------------------
If we had a list that specifically allowed stdbool.h and imagine the compiler
used stopped providing this header, then we may accidentally pick up the system
stdbool.h. Having to change the `.clang-tidy` file in llvm-libc's tree when the
compiler provided includes changes is a maintenance burden and can quickly
become stale.
2. Platform differences
-----------------------
The compiler provided headers may not be the same from operating system to
operating system. This introduces the need for multiple lists for each system
supported, where each list suffers from the above problem.
3. Accidental changes to the include order
------------------------------------------
In situations where a mistake is made in the build system and higher priority
is given to the system includes over the compiler includes. A hand maintained
list would not be able to catch this.
---
Above all else this test needs to be as strict as possible since many of these
situations would allow libc to continue to compile, maybe even pass the tests
but at run-time it could introduce bad behavior due to possible differences in
implementation details. It is important to us to not have a human-point of
failure on this check in my opinion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75332/new/
https://reviews.llvm.org/D75332
Files:
clang-tools-extra/clang-tidy/CMakeLists.txt
clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
clang-tools-extra/docs/clang-tidy/index.rst
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers-transitive.cpp
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t
+
+#include <stdio.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include <stdlib.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdlib.h not allowed
+#include "string.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header string.h not allowed
+#include "stdatomic.h"
+#include <stddef.h>
+// Compiler provided headers should not throw warnings.
Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers-transitive.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers-transitive.cpp
@@ -0,0 +1,6 @@
+// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
+// RUN: -- -header-filter=.* \
+// RUN: -- -I %S/Inputs/llvmlibc
+
+#include "transitive.h"
+// CHECK-MESSAGES: :1:1: warning: system libc header math.h not allowed, transitively included from {{.*}}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h
@@ -0,0 +1 @@
+#include <math.h>
Index: clang-tools-extra/docs/clang-tidy/index.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -68,6 +68,7 @@
``google-`` Checks related to Google coding conventions.
``hicpp-`` Checks related to High Integrity C++ Coding Standard.
``llvm-`` Checks related to the LLVM coding conventions.
+``llvmlibc-`` Checks related to the LLVM-libc coding standards.
``misc-`` Checks that we didn't have a better category for.
``modernize-`` Checks that advocate usage of modern (currently "modern"
means "C++11") language constructs.
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - llvmlibc-restrict-system-libc-headers
+
+llvmlibc-restrict-system-libc-headers
+=====================================
+
+Finds includes of system libc headers not provided by the compiler within
+llvm-libc implementations.
+
+.. code-block:: c++
+
+ #include <stdio.h> // Not allowed because it is part of system libc.
+ #include <stddef.h> // Allowed because it is provided by the compiler.
+ #include "internal/stdio.h" // Allowed because it is NOT part of system libc.
+
+
+This check is necesary because accidentally including sytem 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -187,6 +187,7 @@
`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>`_,
`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>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
Improvements to clang-tidy
--------------------------
+New module
+^^^^^^^^^^
+- New module `llvmlibc`.
+
+ This module contains checks related to the LLVM-libc coding standards.
+
New checks
^^^^^^^^^^
@@ -88,6 +94,11 @@
Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
``memcmp`` and similar derivatives on non-trivial types.
+- New :doc:`llvmlibc-restrict-system-libc-headers
+ <clang-tidy/checks/llvmlibc-restrict-system-libc-headers>` check.
+
+ Finds includes of sytem libc headers in llvm-libc implementation.
+
- New :doc:`objc-dealloc-in-category
<clang-tidy/checks/objc-dealloc-in-category>` check.
Index: clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h
@@ -0,0 +1,35 @@
+//===--- 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"
+
+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 {
+public:
+ RestrictSystemLibcHeadersCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(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
Index: clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
@@ -0,0 +1,75 @@
+//===--- RestrictSystemLibcHeadersCheck.cpp - clang-tidy ------------------===//
+//
+// 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 "RestrictSystemLibcHeadersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm_libc {
+
+namespace {
+
+class RestrictedIncludesPPCallbacks : public PPCallbacks {
+public:
+ explicit RestrictedIncludesPPCallbacks(
+ RestrictSystemLibcHeadersCheck &Check, const SourceManager &SM,
+ const SmallString<128> CompilerIncudeDir)
+ : Check(Check), SM(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)) {
+ DiagnosticBuilder D = Check.diag(
+ HashLoc,
+ "system libc header %0 not allowed, transitively included from %1");
+ D << FileName << SM.getFilename(HashLoc);
+ } else {
+ DiagnosticBuilder D =
+ Check.diag(HashLoc, "system libc header %0 not allowed");
+ D << FileName;
+ }
+ }
+}
+
+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
Index: clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
@@ -0,0 +1,37 @@
+//===--- LLVMLibcTidyModule.cpp - clang-tidy ------------------------------===//
+//
+// 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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "RestrictSystemLibcHeadersCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm_libc {
+
+class LLVMLibcModule : public ClangTidyModule {
+public:
+ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<RestrictSystemLibcHeadersCheck>(
+ "llvmlibc-restrict-system-libc-headers");
+ }
+};
+
+// Register the LLVMLibcTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<LLVMLibcModule>
+ X("llvmlibc-module", "Adds LLVM libc standards checks.");
+
+} // namespace llvm_libc
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the LLVMLibcModule.
+volatile int LLVMLibcModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
@@ -0,0 +1,15 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyLLVMLibcModule
+ LLVMLibcTidyModule.cpp
+ RestrictSystemLibcHeadersCheck.cpp
+
+ LINK_LIBS
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangLex
+ clangTidy
+ clangTidyUtils
+ clangTooling
+ )
Index: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -45,6 +45,11 @@
static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination =
LLVMModuleAnchorSource;
+// This anchor is used to force the linker to link the LLVMLibcModule.
+extern volatile int LLVMLibcModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED LLVMLibcModuleAnchorDestination =
+ LLVMLibcModuleAnchorSource;
+
// This anchor is used to force the linker to link the CppCoreGuidelinesModule.
extern volatile int CppCoreGuidelinesModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -51,6 +51,7 @@
add_subdirectory(hicpp)
add_subdirectory(linuxkernel)
add_subdirectory(llvm)
+add_subdirectory(llvmlibc)
add_subdirectory(misc)
add_subdirectory(modernize)
if(CLANG_ENABLE_STATIC_ANALYZER)
@@ -75,6 +76,7 @@
clangTidyHICPPModule
clangTidyLinuxKernelModule
clangTidyLLVMModule
+ clangTidyLLVMLibcModule
clangTidyMiscModule
clangTidyModernizeModule
clangTidyObjCModule
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
