[libc][clang-tidy] Add llvm-header-guard to get consistant naming and prevent file copy/paste issues. #66477

gchatelet · 2023-09-15T09:09:57Z

No description provided.

gchatelet · 2023-09-15T09:11:56Z

I'll fix the bad headers as well if you agree with this patch.

sivachandra · 2023-09-15T17:42:38Z

This SGTM. We could have done this earlier but there were a few exceptions that we had to deal with so we punted on this, example the include directory. But, what you are proposing is of a much lower scope (restricted to the src directory) so go ahead.

gchatelet · 2023-09-18T07:36:22Z

This SGTM. We could have done this earlier but there were a few exceptions that we had to deal with so we punted on this, example the include directory. But, what you are proposing is of a much lower scope (restricted to the src directory) so go ahead.

Thx for the review @sivachandra . Unfortunately some of the files under src do pull headers in include and so the clang-tidy tries to fix them...
I've had a look at the clang-tidy itself and it already supports customizations for subprojects so I think the correct way forward is to add a customization for llvm-libc as well.

Two questions:

What are the rules for formatting headers in include? (I couldn't find the documentation)
What should we do for directories starting with two underscores? Currently llvm-header-guard suggests the following edits but I think it doesn't really add any value.

>>>
#ifndef LLVM_LIBC_SRC_SUPPORT_CPP_TYPE_TRAITS_IS_NULL_POINTER_H
---
#ifndef LLVM_LIBC_SRC___SUPPORT_CPP_TYPE_TRAITS_IS_NULL_POINTER_H
<<<

gchatelet · 2023-09-18T15:23:12Z

What should we do for directories starting with two underscores?

It seems like libcxx is fine with multiple underscores.

sivachandra · 2023-09-18T19:39:43Z

What are the rules for formatting headers in include? (I couldn't find the documentation)

We are not uniform, but I think we should follow this:

__LLVM_LIBC_<FILE_NAME_IN_UPPER_SNAKE_CASE>_H__

For header files in nested directories:

__LLVM_LIBC_<RELATIVE_PATH_TO_DIR_IN_UPPER_SNAKE_CASE>_<FILE_NAME_IN_UPPER_SNAKE_CASE>_H__

What should we do for directories starting with two underscores? Currently llvm-header-guard suggests the following edits but I think it doesn't really add any value.

Just putting the file name as is, including the double-underscores, seems fine to me. The header guards don't really come in the way of almost anything.

gchatelet · 2023-09-18T20:37:57Z

So llvm-header-guard should not normally flag headers coming from isystem but there were a lot of false positives coming from clang and llvm libc so I took a look at what's going on.

Looking at the compile_commands.json file that is generated by CMake and which drives the clang-tidy it seems that all the headers are appended with the -I option. I believe this is because the headers are added manually via a bunch of

set(include_dirs ${LIBC_SOURCE_DIR} ${LIBC_INCLUDE_DIR})
target_include_directories(${gpu_target_name} PRIVATE ${include_dirs})

instead of using the proper libc-headers target which defines the isystem includes.

Example command line from compile_commands.json shows that all includes are done via -I and none via -isystem

/home/gchatelet/clang_head_install/bin/clang++ -DLIBC_NAMESPACE=__llvm_libc_18_0_0_git -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D_LIBCPP_ENABLE_HARDENED_MODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/gchatelet/build/libc_x86/projects/libc/src/fenv -I/home/gchatelet/git/llvm-project/libc/src/fenv -I/home/gchatelet/build/libc_x86/include -I/home/gchatelet/git/llvm-project/llvm/include -I/home/gchatelet/git/llvm-project/libc -I/home/gchatelet/build/libc_x86/projects/libc/include  -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -march=znver2 -O2 -fpie -ffreestanding -fno-builtin -fno-exceptions -fno-lax-vector-conversions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -Wall -Wextra -Wconversion -Wno-sign-conversion -Wimplicit-fallthrough -Wwrite-strings -Wextra-semi -Wnewline-eof -Wnonportable-system-include-path -Wstrict-prototypes -Wthread-safety -Wglobal-constructors -UNDEBUG -std=c++17 -o projects/libc/src/fenv/CMakeFiles/libc.src.fenv.feclearexcept.__internal__.dir/feclearexcept.cpp.o -c /home/gchatelet/git/llvm-project/libc/src/fenv/feclearexcept.cpp

I may not fully grasp how the header inclusion mechanism works but I believe that -for the purpose of llvm-header-guard - some of the headers should be declared isystem.

Also looking more closely at the above example command line, it seems to me that there are too many includes

-I/home/gchatelet/build/libc_x86/projects/libc/src/fenv
-I/home/gchatelet/git/llvm-project/libc/src/fenv
-I/home/gchatelet/build/libc_x86/include
-I/home/gchatelet/git/llvm-project/llvm/include
-I/home/gchatelet/git/llvm-project/libc
-I/home/gchatelet/build/libc_x86/projects/libc/include

@sivachandra do we expect that many includes?

On the other hand, the Bazel generated command line seems much more reasonable to me

/usr/lib/llvm-14/bin/clang -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer '-std=c++0x' -MD -MF bazel-out/k8-fastbuild/bin/external/llvm-project/libc/_objs/feclearexcept/feclearexcept.pic.d '-frandom-seed=bazel-out/k8-fastbuild/bin/external/llvm-project/libc/_objs/feclearexcept/feclearexcept.pic.o' -fPIC '-DLIBC_NAMESPACE=__llvm_libc_18_0_0_git' -DLIBC_COPT_PUBLIC_PACKAGING '-DLLVM_LIBC_FUNCTION_ATTR=__attribute__((visibility("default")))' '-DBAZEL_CURRENT_REPOSITORY="llvm-project"' -iquote external/llvm-project -iquote bazel-out/k8-fastbuild/bin/external/llvm-project -isystem external/llvm-project/libc -isystem bazel-out/k8-fastbuild/bin/external/llvm-project/libc -O1 -Wall -Wno-deprecated '-std=c++17' -Wno-range-loop-analysis -O3 -fno-builtin -fno-lax-vector-conversions '-fvisibility=hidden' -no-canonical-prefixes -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c external/llvm-project/libc/src/fenv/feclearexcept.cpp -o bazel-out/k8-fastbuild/bin/external/llvm-project/libc/_objs/feclearexcept/feclearexcept.pic.o

sivachandra · 2023-09-18T21:40:04Z

I may not fully grasp how the header inclusion mechanism works but I believe that -for the purpose of llvm-header-guard - some of the headers should be declared isystem.

Your conclusion is correct! We should specify libc/include and/or <build-dir>/projects/libc/include via -isystem.

Also looking more closely at the above example command line, it seems to me that there are too many includes

-I/home/gchatelet/build/libc_x86/projects/libc/src/fenv

-I/home/gchatelet/git/llvm-project/libc/src/fenv

-I/home/gchatelet/build/libc_x86/include

-I/home/gchatelet/git/llvm-project/llvm/include

-I/home/gchatelet/git/llvm-project/libc

-I/home/gchatelet/build/libc_x86/projects/libc/include

@sivachandra do we expect that many includes?

A lot of it is not required. We should only require:

<build-dir>/projects/libc/include
llvm-project/libc/

Anything else seems like unnecessary at this point, but there could some land-mines hiding somewhere. I feel we can take them out with some cleanup.

gchatelet · 2023-09-19T11:22:54Z

Alright so it took me a while but some of these includes come from the main llvm CMakeLists llvm-project/llvm/CMakeLists.txt#L1115.

/home/gchatelet/build/libc_x86/include
/home/gchatelet/git/llvm-project/llvm/include

I don't quite know how to deal with this. We could remove them globally when entering llvm-project/libc/CMakeLists.txt ? 🤷

EDIT: I'm working on a fix, I'll report when I have something that works.

We want to activate `llvm-header-guard` (#66477) but the current CMake configuration includes paths that should be `isystem`. This PR restricts the number of `-I` passed to the clang command line and correctly marks the llvm libc include path as `isystem`.

We want to activate `llvm-header-guard` (llvm#66477) but the current CMake configuration includes paths that should be `isystem`. This PR restricts the number of `-I` passed to the clang command line and correctly marks the llvm libc include path as `isystem`. This is a reland of llvm#66783 a35a3b7 fixing the benchmark breakage.

… prevent file copy/paste issues.

gchatelet · 2023-09-20T11:30:25Z

We are not uniform, but I think we should follow this:

__LLVM_LIBC_<FILE_NAME_IN_UPPER_SNAKE_CASE>_H__

llvm-header-guard derives from header-guard and forbids header guards starting with underscores (original bug).

std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
  // Only reserved identifiers are allowed to start with an '_'.
  return Guard.drop_while([](char C) { return C == '_'; }).str();
}

If we drop this requirement, llvm-header-guard can already be used as is and doesn't need to be adapted to llvm libc.

gchatelet · 2023-09-20T12:21:48Z

libc/src/__support/CPP/cstddef.h

@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//

-#ifndef LLVM_LIBC_SRC_SUPPORT_CPP_BYTE_H
-#define LLVM_LIBC_SRC_SUPPORT_CPP_BYTE_H
+#ifndef LLVM_LIBC_SRC___SUPPORT_CPP_CSTDDEF_H


This is a real error for instance

gchatelet · 2023-09-20T12:30:52Z

libc/src/__support/integer_operations.h

@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//

-#ifndef LLVM_LIBC_SRC_STDLIB_ABS_UTILS_H
-#define LLVM_LIBC_SRC_STDLIB_ABS_UTILS_H
+#ifndef LLVM_LIBC_SRC___SUPPORT_INTEGER_OPERATIONS_H


This one is a real error as well.

gchatelet · 2023-09-20T12:31:54Z

libc/src/__support/threads/callonce.h

@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//

+#ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_CALLONCE_H


This one was missing a header

gchatelet · 2023-09-20T12:34:04Z

libc/src/stdlib/bsearch.h

@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//

+#ifndef LLVM_LIBC_SRC_STDLIB_BSEARCH_H


Missing guard as well

gchatelet · 2023-09-20T12:35:03Z

libc/src/stdlib/div.h

 #ifndef LLVM_LIBC_SRC_STDLIB_DIV_H
 #define LLVM_LIBC_SRC_STDLIB_DIV_H

+#include <stdlib.h>


Guard not at the correct location, same for the following ones.

gchatelet · 2023-09-20T12:36:40Z

libc/src/string/strcasestr.h

@@ -6,13 +6,13 @@
 //
 //===----------------------------------------------------------------------===//

-#ifndef LLVM_LIBC_SRC_STRING_STRCASECMP_H
-#define LLVM_LIBC_SRC_STRING_STRCASECMP_H
+#ifndef LLVM_LIBC_SRC_STRING_STRCASESTR_H


This header is a copy/paste error from libc/src/string/strncasecmp.h

sivachandra · 2023-09-20T14:02:53Z

We are not uniform, but I think we should follow this:
__LLVM_LIBC_<FILE_NAME_IN_UPPER_SNAKE_CASE>_H__

llvm-header-guard derives from header-guard and forbids header guards starting with underscores (original bug).
std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
  // Only reserved identifiers are allowed to start with an '_'.
  return Guard.drop_while([](char C) { return C == '_'; }).str();
}
If we drop this requirement, llvm-header-guard can already be used as is and doesn't need to be adapted to llvm libc.

Clang's freestanding headers use a guard with __CLANG prefix so libc is not being special in that sense. The __ prefix is required so that we do not pollute user namespace.

What is the goal here? Is to use clang-tidy to catch header-guard errors in include/`${LIBC_INCLUDE_DIR}?

gchatelet · 2023-09-20T14:11:21Z

What is the goal here? Is to use clang-tidy to catch header-guard errors in include/`${LIBC_INCLUDE_DIR}?

The goal of this particular patch is to enable clang-tidy to make sure sources under libc/src have properly formatted header guards. Let me know if you believe they should start with __ if so I'll create a new clang-tidy to enforce this rule.

EDIT: Question, the __ prefix is mandatory for headers in both include/* and src/* because the libc can be compiled as part of a wider binary? Or only for include/*?

sivachandra · 2023-09-20T18:29:48Z

The goal of this particular patch is to enable clang-tidy to make sure sources under libc/src have properly formatted header guards. Let me know if you believe they should start with __ if so I'll create a new clang-tidy to enforce this rule.

If include and/or ${LIBC_INCLUDE_DIR} are beyond the scope of this change, then we can discuss that topic separately.

EDIT: Question, the __ prefix is mandatory for headers in both include/* and src/* because the libc can be compiled as part of a wider binary? Or only for include/*?

This is only for the include directory (which means, for ${LIBC_INCLUDE_DIR} directory). Even if libc is compiled as part of the wider binary, the user source code for that binary will not include libc-internal headers and hence we do not have to worry about user code pollution.

gchatelet requested a review from sivachandra September 15, 2023 09:10

gchatelet marked this pull request as draft September 18, 2023 07:36

gchatelet mentioned this pull request Sep 19, 2023

[libc][cmake] Tidy compiler includes #66783

Merged

[libc][clang-tidy] Add llvm-header-guard to get consistant naming and…

6018bc8

… prevent file copy/paste issues.

Change headers as well

ca5c23c

gchatelet force-pushed the add_llvm-header-guard_clang_tidy branch from cb0a4d9 to ca5c23c Compare September 20, 2023 12:19

llvmbot added backend:AMDGPU libc labels Sep 20, 2023

gchatelet commented Sep 20, 2023

View reviewed changes

Fix comment style

3fee72f

gchatelet commented Sep 20, 2023

View reviewed changes

gchatelet marked this pull request as ready for review September 20, 2023 12:38

sivachandra approved these changes Sep 20, 2023

View reviewed changes

gchatelet merged commit 270547f into llvm:main Sep 21, 2023
2 checks passed

gchatelet deleted the add_llvm-header-guard_clang_tidy branch September 21, 2023 09:21

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc][clang-tidy] Add llvm-header-guard to get consistant naming and prevent file copy/paste issues. #66477

[libc][clang-tidy] Add llvm-header-guard to get consistant naming and prevent file copy/paste issues. #66477

gchatelet commented Sep 15, 2023

gchatelet commented Sep 15, 2023

sivachandra commented Sep 15, 2023

gchatelet commented Sep 18, 2023

gchatelet commented Sep 18, 2023

sivachandra commented Sep 18, 2023

gchatelet commented Sep 18, 2023

sivachandra commented Sep 18, 2023

gchatelet commented Sep 19, 2023 •

edited

Loading

gchatelet commented Sep 20, 2023 •

edited

Loading

gchatelet Sep 20, 2023

gchatelet Sep 20, 2023

gchatelet Sep 20, 2023

gchatelet Sep 20, 2023

gchatelet Sep 20, 2023

gchatelet Sep 20, 2023

sivachandra commented Sep 20, 2023

gchatelet commented Sep 20, 2023 •

edited

Loading

sivachandra commented Sep 20, 2023

[libc][clang-tidy] Add llvm-header-guard to get consistant naming and prevent file copy/paste issues. #66477

[libc][clang-tidy] Add llvm-header-guard to get consistant naming and prevent file copy/paste issues. #66477

Conversation

gchatelet commented Sep 15, 2023

gchatelet commented Sep 15, 2023

sivachandra commented Sep 15, 2023

gchatelet commented Sep 18, 2023

gchatelet commented Sep 18, 2023

sivachandra commented Sep 18, 2023

gchatelet commented Sep 18, 2023

sivachandra commented Sep 18, 2023

gchatelet commented Sep 19, 2023 • edited Loading

gchatelet commented Sep 20, 2023 • edited Loading

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

gchatelet Sep 20, 2023

Choose a reason for hiding this comment

sivachandra commented Sep 20, 2023

gchatelet commented Sep 20, 2023 • edited Loading

sivachandra commented Sep 20, 2023

gchatelet commented Sep 19, 2023 •

edited

Loading

gchatelet commented Sep 20, 2023 •

edited

Loading

gchatelet commented Sep 20, 2023 •

edited

Loading