[clang-tidy] `llvmlibc-restrict-system-libc-headers` check erroneously reports on `<stdint.h>`

[jhuber6]

The llvmlibc-restrict-system-libc-headers clang-tidy check will currently report on including the stdint.h header. On many platforms, this is provided by the compiler's resource folder. This file is included in several places in LLVM's libc so we should not be reporting on it. We should test in clang-tidy whether or not the header is not resolved by a file in the resource directory before reporting that it is a system header.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[llvmbot]

@llvm/issue-subscribers-libc

[carlosgalvezp]

@jhuber6 Cannot reproduce on trunk, could you provide more details on your setup?

[jhuber6]

I'm not exactly sure how to reproduce this easily. I've only gotten it when using clangd inside of my editor. Does this reproduce for you?

$ clangd --background-index --clang-tidy --check=./libc/src/__support/RPC/rpc.h 2>&1 | grep 'stdint.h'
E[11:12:19.766] [llvmlibc-restrict-system-libc-headers] Line 24: system include stdint.h not allowed

I'm assuming the problem in this case would be that it's not passing -ffreestanding in this configuration. As without -ffreestanding the stdint.h in the compiler's resource directory will probably #include_next <stdint.h>.

Of course the example you posted doesn't use -ffreestanding either and it doesn't seem to have this problem.

[carlosgalvezp]

Tried to run the command you posted checking on a dummy file /tmp/foo.hpp from a clangd built from source but couldn't repro. I can't get any clang-tidy warning for that matter :man_shrugging: I have never used clangd so it's likely I'm doing something wrong. Could you reproduce on a self-contained file outside the LLVM repo?

[jhuber6]

Yeah it needs to be a source file in the libc directory. It doesn't happen otherwise. if I copy the same file to the current directory it doesn't work and I'm not sure why.

[carlosgalvezp]

Managed to reproduce as follows:

• Configure the LLVM CMake project by adding clang;clang-tools-extra;libc as enabled projects.
• Build clangd.
• Run: ./build/bin/clangd --background-index --clang-tidy --check=./libc/src/__support/RPC/rpc.h --compile-commands-dir=./build

[carlosgalvezp]

After some debugging, I think the relevant code is:

  if (SrcMgr::isSystem(FileType) && SearchPath == CompilerIncudeDir) {
    return;
  }

In a regular clang-tidy test, that branch is taken:

Search path: /path/to/llvm-project/build/release/lib/clang/17/include; CompilerIncudeDir: /path/to/llvm-project/build/release/lib/clang/17/include
Filaname: stdint.h, File type: 1

Whereas in clangd, SearchPath is hardcoded to the string "SearchPath" instead of having a correct value:

Search path: SearchPath; CompilerIncudeDir: /path/to/llvm-project/build/release/lib/clang/17/include
Filaname: stdint.h, File type: 1

Passing -ffreestanding to a clang-tidy invocation still works. I'm not sure what more can be done from clang-tidy's side, I think you should forward the issue to the clangd people to investigate why SearchPath is not set to the correct value.

[carlosgalvezp]

The hardcoded value comes from clangd/ParsedAST.cpp:

      Delegate->InclusionDirective(
          HashTok->location(), SynthesizedIncludeTok, WrittenFilename,
          Inc.Written.front() == '<',
          syntax::FileRange(SM, SynthesizedFilenameTok.getLocation(),
                            SynthesizedFilenameTok.getEndLoc())
              .toCharRange(SM),
          File, "SearchPath", "RelPath",
          /*Imported=*/nullptr, Inc.FileKind);

[llvmbot]

@llvm/issue-subscribers-clangd

[HighCommander4]

I guess clangd should be remembering the search path originally passed in to the InclusionDirective() PP callback so it can pass in the same path when replaying the preamble.

Code was originally added in https://reviews.llvm.org/D54694. I don't see any discussion of the search path during the review, it may been an omission or a shortcut taken at the time.

[kadircet]

Yes, clangd do not preserve those as they were not used by any upstream checks in a meaningful way, so far.

So, one alternative would be to change clangd internals to preserve search/relative path for main file includes. But i'd like to point out that the check actually assumes SearchPath and ResourceDir will be both relative or absolute at the same time, which might not always hold. (e.g. SearchPath can be relative, but ResourceDir might be absolute)

So an alternative fix (which will also get rid of this assumption and stop relying on Search/Relative Path) would be to actually make sure both of them are absolute and file paths don't start with the resource-dir prefix in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp#L51 and use FileEntry::tryGetRealPathName for finding the absolute path of the included files.