This is an archive of the discontinued LLVM Phabricator instance.

Add -Wpoison-system-directories warning
ClosedPublic

Authored by denik on Sep 25 2018, 4:04 PM.

Details

Summary

When using clang as a cross-compiler, we should not use system headers to do the compilation.
This CL adds support of a new warning flag -Wpoison-system-directories which emits warnings if --sysroot is set and headers from common host system location are used.
By default the warning is disabled.

The intention of the warning is to catch bad includes which are usually generated by third party build system not targeting cross-compilation. Such cases happen in Chrome OS when someone imports a new package or upgrade one to a newer version from upstream.

Diff Detail

Event Timeline

yunlian created this revision.Sep 25 2018, 4:04 PM
denik commandeered this revision.Aug 8 2019, 10:00 AM
denik added a reviewer: yunlian.
denik added a subscriber: denik.

Taking ownership of the change as Yunlian is no longer working on this CL.

Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 10:00 AM

Couldn't cross build users just pass -nostdsysteminc to tell clang to not look in system header locations?

denik updated this revision to Diff 215260.Aug 14 2019, 3:00 PM

Updated the code (removed Diag propagation).
Added test cases.

denik updated this revision to Diff 215271.Aug 14 2019, 3:38 PM

Fixed clang-format.

Couldn't cross build users just pass -nostdsysteminc to tell clang to not look in system header locations?

My understanding is "-nostdsysteminc " does not block users from passing include paths from host that are outside sysroot. i.e. clang --sysroot=/foo -nostdsysteminc -I/usr/include still works.
This warning is to catch this behavior.
This is an actual problem in Chrome OS where building some third party packages do not respect cross-compilation.

Wouldn't those projects just move to also disabling the warning by passing -Wno-poison-system-directories? If there are projects that are actively adding -I/usr/include, that means they're consciously fighting the build system and you've kind of already lost, no? Can't you tell them to not use -I/usr/include?

Wouldn't those projects just move to also disabling the warning by passing -Wno-poison-system-directories? If there are projects that are actively adding -I/usr/include, that means they're consciously fighting the build system and you've kind of already lost, no? Can't you tell them to not use -I/usr/include?

Most of the time, those packages are not exactly fighting cross-compilation (e.g. samba), just they just have a broken build system that adds I/usr/include when building them. This warning will help catch bad includes whenever someone imports a new package or upgrade to a newer version from upstream.
Note: in Chrome OS, packages are built using their native build system e.g. LLVM is built with cmake, tensorflow is built with bazel etc.

manojgupta added inline comments.Aug 14 2019, 8:18 PM
clang/include/clang/Basic/DiagnosticGroups.td
1071 ↗(On Diff #215271)

Please verify that the warning is not enabled by default.

Ok, makes sense, thanks for explaining. Please add a summary of that discussion to the patch description / commit message :)

denik updated this revision to Diff 215945.Aug 19 2019, 10:37 AM

Changed Wpoison-system-directories warning to be disabled by default.

denik marked 2 inline comments as done.Aug 19 2019, 10:38 AM
denik added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
1071 ↗(On Diff #215271)

Added DefaultIgnore flag. Now the warning is off by default.

manojgupta added inline comments.Aug 19 2019, 10:49 AM
clang/test/Frontend/warning-poison-system-directories.c
13

Thanks, Can you also add a test for -Werror=poison-system-directories .

denik updated this revision to Diff 215958.Aug 19 2019, 11:31 AM
denik marked 3 inline comments as done.

Manoj, please check updated diff.

clang/test/Frontend/warning-poison-system-directories.c
13

Added the case with -Werror= and expected failure.

Thanks,

Adding a few more reviewers since I am not very familiar with this part of clang.
Please also update the patch description as suggested by @thakis

denik retitled this revision from Add -Wno-poison-system-directories flag to Add -Wpoison-system-directories warning.Aug 19 2019, 4:31 PM
denik edited the summary of this revision. (Show Details)
denik updated this revision to Diff 216021.Aug 19 2019, 4:33 PM

Removed check for libraries.

aaron.ballman added inline comments.Aug 28 2019, 11:42 AM
clang/include/clang/Basic/DiagnosticGroups.td
1072 ↗(On Diff #216021)

Do you envision more warnings being added to this group? If not, I would recommend dropping this change and instead using InGroup<DiagGroup<"poison-system-directories">> in the diagnostic.

clang/lib/Frontend/InitHeaderSearch.cpp
145

I don't think you need the .str() here.

denik updated this revision to Diff 217746.Aug 28 2019, 5:18 PM
denik marked 4 inline comments as done.
denik added a comment.Aug 28 2019, 5:54 PM

Hi Aaron,

Thank you for your review. I updated the diff with suggested changes.

clang/include/clang/Basic/DiagnosticGroups.td
1072 ↗(On Diff #216021)

Added the change. Thanks.

clang/lib/Frontend/InitHeaderSearch.cpp
145

Thank you for pointing out. Fixed.

Ping @aaron.ballman , please verify the change.

aaron.ballman accepted this revision.Sep 11 2019, 12:14 PM

Aside from a minor nit, this LGTM. However, I'm not the most familiar with how cross-compiling works in the first place, so I may be the wrong one to approve this.

clang/lib/Frontend/InitHeaderSearch.cpp
141–143

These should be combined into a single if statement:

if (HasSysroot && (MappedPathStr.startswith(...) || MappedPathStr.startswith(...))) {
This revision is now accepted and ready to land.Sep 11 2019, 12:14 PM
denik updated this revision to Diff 219924.Sep 12 2019, 8:18 AM

Combined two if into one.

denik marked 2 inline comments as done.Sep 12 2019, 8:19 AM
denik added inline comments.
clang/lib/Frontend/InitHeaderSearch.cpp
141–143

Done.

thakis accepted this revision.Sep 12 2019, 2:29 PM
manojgupta closed this revision.Sep 12 2019, 3:43 PM

Submitted as https://reviews.llvm.org/rL371785.
Thanks for the patch!

sbc100 added a subscriber: sbc100.Apr 27 2021, 3:32 PM

Would it make sense to have this apply to system library paths too? i.e. -L/usr/local/lib.

We have a bespoke mechanism in emscripten right now that is a bit more flexible but I'd love to simple replace it with -Wpoison-system-directories.

The name "system directories" seems to imply that it should apply to both include and library directories. It also looks like the gcc version applies to both Warn for -I and -L options using system directories if cross compiling (from https://github.com/Metrological/buildroot/blob/master/package/gcc/4.8.4/910-gcc-poison-system-directories.patch).

Would it make sense to have this apply to system library paths too? i.e. -L/usr/local/lib.

We have a bespoke mechanism in emscripten right now that is a bit more flexible but I'd love to simple replace it with -Wpoison-system-directories.

The name "system directories" seems to imply that it should apply to both include and library directories. It also looks like the gcc version applies to both Warn for -I and -L options using system directories if cross compiling (from https://github.com/Metrological/buildroot/blob/master/package/gcc/4.8.4/910-gcc-poison-system-directories.patch).

FWIW, I think it would make sense to support library paths as well.

An earlier version did check for library directories [1]. I am not exactly sure why was it removed, maybe it didn't work. So if anyone is willing to test that, please apply the diff and try.

[1] Diff https://reviews.llvm.org/D52524?id=215958