[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeb41cc619866: [clang-tidy] Add module for llvm-libc and 
restrict-system-libc-header-check. (authored by PaulkaToast).

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/resource/include/stdatomic.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/stddef.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/math.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdio.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/string.h
  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,13 @@
+// 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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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,8 @@
+// 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 {{.*}}
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 
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 // Not allowed because it is part of system libc.
+   #include// Allowed because it is provided by the compiler.
+   #include "internal/stdio.h"   // Allowed because it is NOT part o

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 249802.
PaulkaToast added a comment.

Mocked the header files so that we don't experience failures due to differences 
in systems. Mind taking a quick look @aaron.ballman?


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/resource/include/stdatomic.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/stddef.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/math.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdio.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/string.h
  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,13 @@
+// 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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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,8 @@
+// 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 {{.*}}
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 
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 // Not allowed because it is part of system libc.
+   #include// Allowed because it is provided by the compiler.
+   #include "internal/stdio.h"   // Allowed because it is NOT par

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:66-67
+const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
+  SmallString<128> CompilerIncudeDir =
+  StringRef(PP->getHeaderSearchInfo().getHeaderSearchOpts().ResourceDir);
+  llvm::sys::path::append(CompilerIncudeDir, "include");

PaulkaToast wrote:
> aaron.ballman wrote:
> > The user can control this path -- is that an issue? You're using it to 
> > determine what a compiler-provided header file is, and this seems like an 
> > escape hatch for users to get around that. If that's reasonable to you, 
> > then I'm okay with it, but you had mentioned you want to remove human error 
> > as a factor and this seems like it could be a subtle human error situation.
> Ah, thanks for pointing this out! I didn't consider this. I feel like 
> scenario is a more unlikely then situations I mentioned. Probably not 
> something to be too concerned about unless you know of a way to get the 
> default resource path?
I also don't think it's particularly likely people will want to use this just 
to silence diagnostics, so I think it's fine as-is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 249544.
PaulkaToast added a comment.

Addressed @Eugene.Zelenko comments.


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,14 @@
+// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t
+// We dont know which headers are provided on each system.
+// Therefore, we only test on linux.
+// UNSUPPORTED: !linux
+
+#include 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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,9 @@
+// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
+// RUN:   -- -header-filter=.* \
+// RUN:   -- -I %S/Inputs/llvmlibc
+// We dont know which headers are provided on each system.
+// Therefore, we only test on linux.
+// UNSUPPORTED: !linux
+
+#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 
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 // Not allowed because it is part of system libc.
+   #include// Allowed because it is provided by the compiler.
+   #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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
++

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:100
+
+  Finds includes of sytem libc headers in llvm-libc implementation.
+

Please synchronize with first statement in documentation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:66-67
+const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
+  SmallString<128> CompilerIncudeDir =
+  StringRef(PP->getHeaderSearchInfo().getHeaderSearchOpts().ResourceDir);
+  llvm::sys::path::append(CompilerIncudeDir, "include");

aaron.ballman wrote:
> The user can control this path -- is that an issue? You're using it to 
> determine what a compiler-provided header file is, and this seems like an 
> escape hatch for users to get around that. If that's reasonable to you, then 
> I'm okay with it, but you had mentioned you want to remove human error as a 
> factor and this seems like it could be a subtle human error situation.
Ah, thanks for pointing this out! I didn't consider this. I feel like scenario 
is a more unlikely then situations I mentioned. Probably not something to be 
too concerned about unless you know of a way to get the default resource path?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 249530.
PaulkaToast marked 4 inline comments as done.
PaulkaToast added a comment.

Addressed @aaron.ballman comments (:


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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 // Not allowed because it is part of system libc.
+   #include// Allowed because it is provided by the compiler.
+   #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.
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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:52
+if (!SM.isInMainFile(HashLoc)) {
+  DiagnosticBuilder D = Check.diag(
+  HashLoc,

No need for the local DiagnosticBuilder object, you can stream into the 
Check.diag() call.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:57
+} else {
+  DiagnosticBuilder D =
+  Check.diag(HashLoc, "system libc header %0 not allowed");

No need for the local `DiagnosticBuilder` object, you can stream into the 
`Check.diag()` call.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:66-67
+const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
+  SmallString<128> CompilerIncudeDir =
+  StringRef(PP->getHeaderSearchInfo().getHeaderSearchOpts().ResourceDir);
+  llvm::sys::path::append(CompilerIncudeDir, "include");

The user can control this path -- is that an issue? You're using it to 
determine what a compiler-provided header file is, and this seems like an 
escape hatch for users to get around that. If that's reasonable to you, then 
I'm okay with it, but you had mentioned you want to remove human error as a 
factor and this seems like it could be a subtle human error situation.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:16
+
+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

necesary -> necessary


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

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 .

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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D75332#1904317 , @MaskRay wrote:

> In D75332#1904264 , @PaulkaToast 
> wrote:
>
> > In D75332#1903570 , @MaskRay wrote:
> >
> > > > `RestrictSystemLibcHeadersCheck`
> > >
> > > As I commented previously, I think the checker name should be 
> > > generalized, as it does not need to be coupled with llvm-libc. Other 
> > > projects may have similar needs. For example, they don't want to 
> > > accidentally include a system zlib.h -> they may ship a bundled zlib 
> > > (say, in third_party/zlib/).
> > >
> > > Maybe `misc/` (or `portability/`) is more suitable?
> >
> >
> > This is a simple check made precisely for llvm libc's use-case and doesn't 
> > require a human maintained list. As I mentioned, if a more 
> > general/configurable check is desired then porting out the 
> > fuchsia-restrict-system-includes would make more sense as it already 
> > implements white-lists and would handle the situation you described.
>
>
>
>
> 1. D43778  fuchsia-restrict-system-includes
> 2. This patch llvmlibc-restrict-system-libc-headers
> 3. The zlib use case I mentioned.
>
>   I think we should consider generalizing the existing 
> fuchsia-restrict-system-includes (I did not know it). +@juliehockett
>
>   Fuchsia and llvm-libc developers can use a config once the general checker 
> is ready.


+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 248059.
PaulkaToast marked an inline comment as done.

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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 // Not allowed because it is part of system libc.
+   #include// 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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-restrict-system-libc-headers `_,
`mi

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a subscriber: juliehockett.
MaskRay added a comment.

In D75332#1904264 , @PaulkaToast wrote:

> In D75332#1903570 , @MaskRay wrote:
>
> > > `RestrictSystemLibcHeadersCheck`
> >
> > As I commented previously, I think the checker name should be generalized, 
> > as it does not need to be coupled with llvm-libc. Other projects may have 
> > similar needs. For example, they don't want to accidentally include a 
> > system zlib.h -> they may ship a bundled zlib (say, in third_party/zlib/).
> >
> > Maybe `misc/` (or `portability/`) is more suitable?
>
>
> This is a simple check made precisely for llvm libc's use-case and doesn't 
> require a human maintained list. As I mentioned, if a more 
> general/configurable check is desired then porting out the 
> fuchsia-restrict-system-includes would make more sense as it already 
> implements white-lists and would handle the situation you described.




1. D43778  fuchsia-restrict-system-includes
2. This patch llvmlibc-restrict-system-libc-headers
3. The zlib use case I mentioned.

I think we should consider generalizing the existing 
fuchsia-restrict-system-includes (I did not know it). +@juliehockett

Fuchsia and llvm-libc developers can use a config once the general checker is 
ready.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:18
+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

Please use double-ticks for language constructs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added a comment.

In D75332#1903570 , @MaskRay wrote:

> > `RestrictSystemLibcHeadersCheck`
>
> As I commented previously, I think the checker name should be generalized, as 
> it does not need to be coupled with llvm-libc. Other projects may have 
> similar needs. For example, they don't want to accidentally include a system 
> zlib.h -> they may ship a bundled zlib (say, in third_party/zlib/).
>
> Maybe `misc/` (or `portability/`) is more suitable?


This is a simple check made precisely for llvm libc's use-case and doesn't 
require a human maintained list. As I mentioned, if a more general/configurable 
check is desired then porting out the fuchsia-restrict-system-includes would 
make more sense as it already implements white-lists and would handle the 
situation you described.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 248044.
PaulkaToast marked 2 inline comments as done.

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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 // Not allowed because it is part of system libc.
+   #include// 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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-restrict-system-libc-headers `_,
`misc

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:16-20
+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 `FILE *` 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.

abrachet wrote:
> To be annoyingly pedantic, `FILE` is opaque so it would actually be ok in 
> this situation. It is undefined to allocate your own `FILE` I believe. 
> Something like `jmp_buf` or `thrd_t` might be better examples? Or macros like 
> `assert` or `errno`.
`FILE` is a problem of glibc. Other libcs don't necessarily have the problem.

glibc exposes the internals of `FILE`: `typedef struct _IO_FILE FILE;` and 
`struct _IO_FILE { ... }` is exposed. musl does not expose the underlying 
structure of `FILE` (POSIX intentionally does not require that to allow a 
flexible implementation).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 247960.
PaulkaToast marked 2 inline comments as done.

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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 // Not allowed because it is part of system libc.
+   #include// 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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-restrict-system-libc-headers `_,
`misc

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> `RestrictSystemLibcHeadersCheck`

As I commented previously, I think the checker name should be generalized, as 
it does not need to be coupled with llvm-libc. Other projects may have similar 
needs. For example, they don't want to accidentally include a system zlib.h -> 
they may ship a bundled zlib (say, in third_party/zlib/).

Maybe `misc/` (or `portability/`) is more suitable?




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:49
+  if (SrcMgr::isSystem(FileType)) {
+// Compiler provided headers are allowed (e.g stddef.h).
+if (SearchPath == CompilerIncudeDir) {

This is actually an interesting topic.

On FreeBSD and linux-musl, stdalign.h stdarg.h stdbool.h stddef.h stdint.h 
stdnoreturn.h are provided by libc instead.

After D65699,

```
% clang -fsyntax-only -target x86_64-linux-musl -v /tmp/c/a.c
...
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include
 /tmp/ReleaseA/lib/clang/11.0.0/include
End of search list.
```

Note that the $resource_dir/include is after libc search directories on a Linux 
glibc (x86_64-linux-gnu) system.


Using a compiler intrinsic file (e.g. `x86intrin.h`) may be less controversial. 
It leaves to llvm-libc to decide which ordering is better. (I lean toward 
FreeBSD/linux musl way.)



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:52
+  return;
+}
+if (!SM.isInMainFile(HashLoc)) {

Drop excess parentheses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Alex Brachet via Phabricator via cfe-commits]

abrachet added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+SrcMgr::CharacteristicKind FileType) {
+  if (SrcMgr::isSystem(FileType)) {
+if (!SM.isInMainFile(HashLoc)) {

PaulkaToast wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > abrachet wrote:
> > > > Could you whitelist the freestanding/compiler provided headers like 
> > > > stddef, stdatomic...
> > > Or have a user configurable whitelist 
> > It should be configurable and named something other than whitelist. I'd 
> > recommend `AllowedHeaders` or something along those lines.
> Maintaining a list of acceptable and blacklisted headers would produce a fair 
> bit of maintenance burden. We have a specific use-case in mind here for 
> llvm-libc which is to avoid use of system libc headers that aren't provided 
> by the compiler. I've added a check to allow for compiler provided headers 
> without necessitating a black/white list. 
> 
> If a general check is desirable then my suggestion is to pull out [[ 
> https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-restrict-system-includes.html
>  | fuchsia-restrict-system-includes ]] which already implements the features 
> mentioned.
> I've added a check to allow for compiler provided headers without 
> necessitating a black/white list.

This is a much better solution for sure.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:11-13
+   #include // Not allowed because it is part of system 
libc
+   #include// Allowed because it is provided by the 
compiler
+   #include "internal/stdio.h"   // Allowed because it is NOT part of system 
libc

Nit: We do periods at the end of comments in real code so for parity I think it 
makes sense here too.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:16-20
+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 `FILE *` 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.

To be annoyingly pedantic, `FILE` is opaque so it would actually be ok in this 
situation. It is undefined to allocate your own `FILE` I believe. Something 
like `jmp_buf` or `thrd_t` might be better examples? Or macros like `assert` or 
`errno`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added a comment.

In D75332#1897487 , @njames93 wrote:

> The test cases need fixing as they are causing the build to fail.


Done.

> Also would it be wise to add a .clang-tidy file to libc/ to enable this 
> module for that subdirectory?

Yes, this will be done in a separate patch. Thanks for pointing it out!

In D75332#1897868 , @Eugene.Zelenko 
wrote:

> Please mention new module in docs/clang-tidy/index.rst and Release Notes (new 
> modules section is above new checks one and please add subsubsection).


Done.

In D75332#1899201 , @MaskRay wrote:

> I am of the feeling that this check should not be llvm-libc specific. It is a 
> general need that certain system headers are not desired. This patch can 
> provide a general mechanism (some whitelists and blacklists).


Please see my reply to aaron.




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+SrcMgr::CharacteristicKind FileType) {
+  if (SrcMgr::isSystem(FileType)) {
+if (!SM.isInMainFile(HashLoc)) {

aaron.ballman wrote:
> njames93 wrote:
> > abrachet wrote:
> > > Could you whitelist the freestanding/compiler provided headers like 
> > > stddef, stdatomic...
> > Or have a user configurable whitelist 
> It should be configurable and named something other than whitelist. I'd 
> recommend `AllowedHeaders` or something along those lines.
Maintaining a list of acceptable and blacklisted headers would produce a fair 
bit of maintenance burden. We have a specific use-case in mind here for 
llvm-libc which is to avoid use of system libc headers that aren't provided by 
the compiler. I've added a check to allow for compiler provided headers without 
necessitating a black/white list. 

If a general check is desirable then my suggestion is to pull out [[ 
https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-restrict-system-includes.html
 | fuchsia-restrict-system-includes ]] which already implements the features 
mentioned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 247794.
PaulkaToast marked 11 inline comments as done.
Herald added subscribers: jfb, arphaman.

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 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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 
+// 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 
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 // Not allowed because it is part of system libc
+   #include// 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 `FILE *` 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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-res

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+SrcMgr::CharacteristicKind FileType) {
+  if (SrcMgr::isSystem(FileType)) {
+if (!SM.isInMainFile(HashLoc)) {

njames93 wrote:
> abrachet wrote:
> > Could you whitelist the freestanding/compiler provided headers like stddef, 
> > stdatomic...
> Or have a user configurable whitelist 
It should be configurable and named something other than whitelist. I'd 
recommend `AllowedHeaders` or something along those lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+SrcMgr::CharacteristicKind FileType) {
+  if (SrcMgr::isSystem(FileType)) {
+if (!SM.isInMainFile(HashLoc)) {

abrachet wrote:
> Could you whitelist the freestanding/compiler provided headers like stddef, 
> stdatomic...
Or have a user configurable whitelist 



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:42
+if (!SM.isInMainFile(HashLoc)) {
+  auto D = Check.diag(
+  HashLoc,

Don't use auto when the type isn't an iterator or spelled out on the 
initialisation. 



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:47
+} else {
+  auto D = Check.diag(HashLoc, "system libc header %0 not allowed");
+  D << FileName;

Ditto


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I am of the feeling that this check should not be llvm-libc specific. It is a 
general need that certain system headers are not desired. This patch can 
provide a general mechanism (some whitelists and blacklists).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Alex Brachet via Phabricator via cfe-commits]

abrachet added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+SrcMgr::CharacteristicKind FileType) {
+  if (SrcMgr::isSystem(FileType)) {
+if (!SM.isInMainFile(HashLoc)) {

Could you whitelist the freestanding/compiler provided headers like stddef, 
stdatomic...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new module in docs/clang-tidy/index.rst and Release Notes (new 
modules section is above new checks one and please add subsubsection).




Comment at: clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp:21
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+
+CheckFactories.registerCheck(

Unnecessary empty line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

The test cases need fixing as they are causing the build to fail. Also would it 
be wise to add a .clang-tidy file to libc/ to enable this module for that 
subdirectory?




Comment at: clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt:17
+  
\ No newline at end of file


Not sure why this error is here but can it be resolved



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:17
+
+class RestrictedIncludesPPCallbacks : public PPCallbacks {
+public:

Should put this in an anonymous namespace as it doesn't need external linkage



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h:18
+
+/// FIXME: Write a short description.
+///

FIXME



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:6
+
+Finds includes of system libc headers within llvm-libc implementations.
+

Should explain the rationale for this check


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast created this revision.
PaulkaToast added a reviewer: sivachandra.
PaulkaToast added projects: clang-tools-extra, libc-project.
Herald added subscribers: cfe-commits, MaskRay, xazax.hun, mgorny.
Herald added a project: clang.

This adds a new module to enforce standards specific to the llvm-libc project. 
This change also adds the first check which restricts user from including 
system libc headers accidentally which can lead to subtle bugs that would be a 
challenge to detect.


Repository:
  rG LLVM Github Monorepo

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/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h
  
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,12 @@
+// 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 {{.*}}
+#include 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system libc header stdio.h not allowed
+#include 
+// 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
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 
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,11 @@
+.. title:: clang-tidy - llvmlibc-restrict-system-libc-headers
+
+llvmlibc-restrict-system-libc-headers
+=
+
+Finds includes of system libc headers within llvm-libc implementations.
+
+.. code-block:: c++
+
+   #include // illegal include of system libc header
+   #include "internal/stdio.h"
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 `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-restrict-system-libc-headers `_,
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
`misc-new-delete-overloads `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -88,6 +88,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
+  ` check.
+
+  Finds includes of sytem libc headers in llvm-libc implementation.
+
 - New :doc:`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,34 @@
+//===--- 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 {
+namespac