Add -Wpoison-system-directories warning

[denik]

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.

[denik]

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

[thakis]

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

[denik]

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

[denik]

Fixed clang-format.

[manojgupta]

In D52524#1621468, @thakis wrote:

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.

[thakis]

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?

[manojgupta]

In D52524#1630767, @thakis wrote:

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]

clang/include/clang/Basic/DiagnosticGroups.td

1071 ↗

(On Diff #215271)

Please verify that the warning is not enabled by default.

[thakis]

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

[denik]

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

[denik]

clang/include/clang/Basic/DiagnosticGroups.td

1071 ↗

(On Diff #215271)

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

[manojgupta]

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

13

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

[denik]

Manoj, please check updated diff.

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

13

Added the case with -Werror= and expected failure.

[manojgupta]

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]

Removed check for libraries.

[aaron.ballman]

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]

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.

[denik]

Ping @aaron.ballman , please verify the change.

[aaron.ballman]

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

[denik]

Combined two if into one.

[denik]

clang/lib/Frontend/InitHeaderSearch.cpp

141–143

Done.

[manojgupta]

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

[sbc100]

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

[aaron.ballman]

In D52524#2721073, @sbc100 wrote:

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.

[manojgupta]

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

clang/include/clang/Basic/DiagnosticCommonKinds.td

 def err_omp_more_one_clause : Error<
   "directive '#pragma omp %0' cannot contain more than one '%1' clause%select{| with '%3' name modifier| with 'source' dependence}2">;
 
 // Static Analyzer Core
 def err_unknown_analyzer_checker_or_package : Error<
     "no analyzer checkers or packages are associated with '%0'">;
 def note_suggest_disabling_all_checkers : Note<
     "use -analyzer-disable-all-checks to disable all static analyzer checkers">;
+
+// Poison system directories.
+def warn_poison_system_directories : Warning <
+  "include location '%0' is unsafe for cross-compilation">,
+  InGroup<DiagGroup<"poison-system-directories">>, DefaultIgnore;
 }

clang/lib/Frontend/InitHeaderSearch.cpp

 bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group,
                                        bool isFramework) {
   assert(!Path.isTriviallyEmpty() && "can't handle empty path here");
 
   FileManager &FM = Headers.getFileMgr();
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers while cross-compiling, emit the warning.
+  if (HasSysroot && (MappedPathStr.startswith("/usr/include") ||
+                     MappedPathStr.startswith("/usr/local/include"))) {
+    Headers.getDiags().Report(diag::warn_poison_system_directories)
+        << MappedPathStr;
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
     Type = SrcMgr::C_User;
   } else if (Group == ExternCSystem) {
     Type = SrcMgr::C_ExternCSystem;
   } else {
     Type = SrcMgr::C_System;

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

+// System directory and sysroot option causes warning.
+// RUN: %clang -Wpoison-system-directories -target x86_64 -I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 -cxx-isystem/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 -iquote/usr/local/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 -isystem/usr/local/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+
+// Missing target but included sysroot still causes the warning.
+// RUN: %clang -Wpoison-system-directories -I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.2.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.2.stderr %s
+
+// With -Werror the warning causes the failure.
+// RUN: not %clang -Werror=poison-system-directories -target x86_64 -I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.3.stderr
+// RUN: FileCheck -check-prefix=ERROR < %t.3.stderr %s
+
+// Cros target without sysroot causes no warning.
+// RUN: %clang -Wpoison-system-directories -Werror -target x86_64 -I/usr/include -c -o - %s
+
+// By default the warning is off.
+// RUN: %clang -Werror -target x86_64 -I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s
+
+// WARN: warning: include location {{[^ ]+}} is unsafe for cross-compilation [-Wpoison-system-directories]
+
+// ERROR: error: include location {{[^ ]+}} is unsafe for cross-compilation [-Werror,-Wpoison-system-directories]