This is an archive of the discontinued LLVM Phabricator instance.

[libc] Fix accidental inclusion of system libc headers.
ClosedPublic

Authored by PaulkaToast on May 18 2020, 9:25 PM.

Details

Summary

I found that because --system-headers flag was not included when running clang-tidy, errors produced from compiler provided headers were being suppressed. After passing this flag I realized that by including headers like stdint.h we were indirectly including headers from the system libc. To prevent this we pass -ffreestanding.
We don't want to pass --system-headers for all checks just the llvmlibc-restrict-system-libc-headers therefore we do a separate invocation of clang-tidy for this check.

Diff Detail

Event Timeline

PaulkaToast created this revision.May 18 2020, 9:25 PM

Doesn't this trigger a "defining builtin macro" warning/error?
Does adding -ffreestanding to the compile step of the raw object file fix the problem you are seeing and also avoid tampering with __STDC_HOSTED__ macro?

Doesn't this trigger a "defining builtin macro" warning/error?
Does adding -ffreestanding to the compile step of the raw object file fix the problem you are seeing and also avoid tampering with __STDC_HOSTED__ macro?

Ah -ffreestanding works as well and its much cleaner. Thanks for the recommendation!

PaulkaToast edited the summary of this revision. (Show Details)May 18 2020, 10:26 PM
sivachandra added inline comments.May 20 2020, 10:01 PM
libc/cmake/modules/LLVMLibCObjectRules.cmake
220

Do we still need this?
Normally, it should not be our business to check system headers. For example, when we add the LLVM header guard check, system headers will break?

PaulkaToast marked an inline comment as done.May 20 2020, 10:59 PM
PaulkaToast added inline comments.
libc/cmake/modules/LLVMLibCObjectRules.cmake
220

Ah yes, this breaks with something like llvm-header-guard.

The restrict-system-libc-header check needs the --system-headers to ensure that we aren't transitively including through the compiler headers. To solve this we can use two invocations of clang-tidy, one that just runs the restrict-system-libc-header check with the --system-headers and the other invocation would run everything else.

sivachandra accepted this revision.May 20 2020, 11:11 PM
sivachandra added inline comments.
libc/cmake/modules/LLVMLibCObjectRules.cmake
220

Add this as a TODO and include the full reasoning you mentioned.

This revision is now accepted and ready to land.May 20 2020, 11:11 PM
sivachandra accepted this revision.May 21 2020, 12:00 AM

Please update and format the commit message appropriately.

PaulkaToast edited the summary of this revision. (Show Details)May 21 2020, 12:08 AM
This revision was automatically updated to reflect the committed changes.