Page MenuHomePhabricator

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

Authored by PaulkaToast on Feb 28 2020, 12:27 AM.

Details

Summary

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.

Diff Detail

Event Timeline

PaulkaToast created this revision.Feb 28 2020, 12:27 AM

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?

clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
16

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

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
18

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

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h
19

FIXME

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
7

Should explain the rationale for this check

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).

clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
22

Unnecessary empty line.

Eugene.Zelenko added a subscriber: sivachandra.
abrachet added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
41

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

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).

njames93 added inline comments.Mar 1 2020, 1:56 AM
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
41

Or have a user configurable whitelist

43

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

48

Ditto

aaron.ballman added inline comments.Mar 2 2020, 7:38 AM
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
41

It should be configurable and named something other than whitelist. I'd recommend AllowedHeaders or something along those lines.

PaulkaToast marked 11 inline comments as done.

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!

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.

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.

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
41

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 fuchsia-restrict-system-includes which already implements the features mentioned.

abrachet added inline comments.Mar 3 2020, 1:06 AM
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
41

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.

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
12–14

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

17–21

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.

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?

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
50

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.)

53

Drop excess parentheses.

MaskRay added inline comments.Mar 3 2020, 10:47 AM
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
17–21

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).

PaulkaToast marked 2 inline comments as done.
PaulkaToast marked 2 inline comments as done.

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.

MaskRay added a subscriber: juliehockett.EditedMar 3 2020, 3:06 PM

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.

Eugene.Zelenko added inline comments.Mar 3 2020, 3:06 PM
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
19

Please use double-ticks for language constructs.

PaulkaToast marked an inline comment as done.
PaulkaToast marked an inline comment as done.

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

PaulkaToast updated this revision to Diff 248851.EditedMar 6 2020, 4:24 PM

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.

aaron.ballman added inline comments.Mar 10 2020, 1:12 PM
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
53

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

58

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

67–68

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.

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
17

necesary -> necessary

PaulkaToast marked 4 inline comments as done.

Addressed @aaron.ballman comments (:

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
67–68

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?

Eugene.Zelenko added inline comments.Mar 10 2020, 6:46 PM
clang-tools-extra/docs/ReleaseNotes.rst
107

Please synchronize with first statement in documentation.

PaulkaToast marked an inline comment as done.Mar 10 2020, 7:27 PM
aaron.ballman accepted this revision.Mar 11 2020, 11:28 AM

LGTM!

clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
67–68

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.

This revision is now accepted and ready to land.Mar 11 2020, 11:28 AM

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

This revision was automatically updated to reflect the committed changes.