[PATCH] D52524: Add -Wno-poison-system-directories flag

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added reviewers: rtrieu, aaron.ballman.
manojgupta added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik added a comment.

Manoj, please check updated diff.




Comment at: clang/test/Frontend/warning-poison-system-directories.c:12
+// 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

manojgupta wrote:
> Thanks, Can you also add a test for  -Werror=poison-system-directories .
Added the case with -Werror= and expected failure. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik updated this revision to Diff 215958.
denik marked 3 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/lib/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/include/c++/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/lib/gcc/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/include/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/lib/.keep
  clang/test/Frontend/warning-poison-system-directories.c


Index: clang/test/Frontend/warning-poison-system-directories.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directories.c
@@ -0,0 +1,27 @@
+// 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]
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -137,6 +137,18 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers/libraries while cross-compiling,
+  // emit the warning.
+  if (HasSysroot) {
+if (MappedPathStr.startswith("/usr/include") ||
+MappedPathStr.startswith("/usr/local/include") ||
+MappedPathStr.startswith("/lib") ||
+MappedPathStr.startswith("/usr/local/lib")) {
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr.str();
+}
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,6 +1066,10 @@
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
 
+// A warning group for warnings about including system headers when
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -304,4 +304,9 @@
 "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 s

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.



Comment at: clang/test/Frontend/warning-poison-system-directories.c:12
+// 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

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik marked 2 inline comments as done.
denik added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1071
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+

manojgupta wrote:
> Please verify that the warning is not enabled by default.
Added DefaultIgnore flag. Now the warning is off by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik updated this revision to Diff 215945.
denik added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/lib/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/include/c++/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/lib/gcc/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/include/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/lib/.keep
  clang/test/Frontend/warning-poison-system-directories.c


Index: clang/test/Frontend/warning-poison-system-directories.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directories.c
@@ -0,0 +1,21 @@
+// 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
+
+// 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]
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -137,6 +137,18 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers/libraries while cross-compiling,
+  // emit the warning.
+  if (HasSysroot) {
+if (MappedPathStr.startswith("/usr/include") ||
+MappedPathStr.startswith("/usr/local/include") ||
+MappedPathStr.startswith("/lib") ||
+MappedPathStr.startswith("/usr/local/lib")) {
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr.str();
+}
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,6 +1066,10 @@
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
 
+// A warning group for warnings about including system headers when
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -304,4 +304,9 @@
 "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, DefaultIgnore;
 }


Index: clang/test/Frontend/warning-poison-system-directories.c
===
--- /dev/null
+++ clang/test/Frontend/warni

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1071
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+

Please verify that the warning is not enabled by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik updated this revision to Diff 215271.
denik added a comment.

Fixed clang-format.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/lib/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/include/c++/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/lib/gcc/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/include/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/lib/.keep
  clang/test/Frontend/warning-poison-system-directory.c


Index: clang/test/Frontend/warning-poison-system-directory.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directory.c
@@ -0,0 +1,18 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -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 -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 -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 -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 -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
+
+// Cros target without sysroot causes no warning.
+// RUN: %clang -Werror -target x86_64 -I/usr/include -c -o - %s
+
+// WARN: warning: include location {{[^ ]+}} is unsafe for cross-compilation 
[-Wpoison-system-directories]
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -137,6 +137,18 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers/libraries while cross-compiling,
+  // emit the warning.
+  if (HasSysroot) {
+if (MappedPathStr.startswith("/usr/include") ||
+MappedPathStr.startswith("/usr/local/include") ||
+MappedPathStr.startswith("/lib") ||
+MappedPathStr.startswith("/usr/local/lib")) {
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr.str();
+}
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,6 +1066,10 @@
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
 
+// A warning group for warnings about including system headers when
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -304,4 +304,7 @@
 "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;
 }


Index: clang/test/Frontend/warning-poison-system-directory.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directory.c
@@ -0,0 +1,18 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -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 -target x86_64 -cxx-isystem/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik updated this revision to Diff 215260.
denik added a comment.
Herald added a subscriber: ormris.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/lib/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/include/c++/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/lib/gcc/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/include/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/lib/.keep
  clang/test/Frontend/warning-poison-system-directory.c


Index: clang/test/Frontend/warning-poison-system-directory.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directory.c
@@ -0,0 +1,18 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -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 -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 -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 -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 -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
+
+// Cros target without sysroot causes no warning.
+// RUN: %clang -Werror -target x86_64 -I/usr/include -c -o - %s
+
+// WARN: warning: include location {{[^ ]+}} is unsafe for cross-compilation 
[-Wpoison-system-directories]
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -137,6 +137,17 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers/libraries while cross-compiling,
+  // emit the warning.
+  if (HasSysroot) {
+if(MappedPathStr.startswith("/usr/include") ||
+   MappedPathStr.startswith("/usr/local/include") ||
+   MappedPathStr.startswith("/lib") ||
+   MappedPathStr.startswith("/usr/local/lib")) {
+ Headers.getDiags().Report(diag::warn_poison_system_directories) << 
MappedPathStr.str();
+   }
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,6 +1066,10 @@
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
 
+// A warning group for warnings about including system headers when
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -304,4 +304,7 @@
 "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;
 }


Index: clang/test/Frontend/warning-poison-system-directory.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directory.c
@@ -0,0 +1,18 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -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 -target x86_64 -cxx-isystem/usr/include

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik commandeered this revision.
denik added a reviewer: yunlian.
denik added a comment.
Herald added a project: clang.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52524/new/

https://reviews.llvm.org/D52524



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Yunlian Jiang via Phabricator via cfe-commits]

yunlian created this revision.
Herald added a subscriber: cfe-commits.

When using clang as a cross-compiler, we should not use system headers or 
libraries to do the compilation. This CL creates a new warning flag
-Wpoison-system-directories support to emit warnings if --sysroot is set and 
headers or libraries from common host system location are used.


Repository:
  rC Clang

https://reviews.llvm.org/D52524

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Frontend/Utils.h
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/InitHeaderSearch.cpp


Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -45,12 +45,13 @@
   HeaderSearch &Headers;
   bool Verbose;
   std::string IncludeSysroot;
+  DiagnosticsEngine &Diag;
   bool HasSysroot;
 
 public:
 
-  InitHeaderSearch(HeaderSearch &HS, bool verbose, StringRef sysroot)
-: Headers(HS), Verbose(verbose), IncludeSysroot(sysroot),
+  InitHeaderSearch(HeaderSearch &HS, bool verbose, StringRef sysroot, 
DiagnosticsEngine &Diag)
+: Headers(HS), Verbose(verbose), IncludeSysroot(sysroot), Diag(Diag),
   HasSysroot(!(sysroot.empty() || sysroot == "/")) {
   }
 
@@ -138,6 +139,17 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers/libraries while cross-compiling,
+  // emit the warning.
+  if (HasSysroot) {
+if(MappedPathStr.startswith("/usr/include") ||
+   MappedPathStr.startswith("/usr/local/include") ||
+   MappedPathStr.startswith("/lib") ||
+   MappedPathStr.startswith("/usr/local/lib") {
+ Diag.Report(diag::warn_poison_system_directories) << 
MappedPathStr.str();
+   }
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
@@ -663,8 +675,9 @@
 void clang::ApplyHeaderSearchOptions(HeaderSearch &HS,
  const HeaderSearchOptions &HSOpts,
  const LangOptions &Lang,
+ DiagnosticsEngine &Diag,
  const llvm::Triple &Triple) {
-  InitHeaderSearch Init(HS, HSOpts.Verbose, HSOpts.Sysroot);
+  InitHeaderSearch Init(HS, HSOpts.Verbose, HSOpts.Sysroot, Diag);
 
   // Add the user defined entries.
   for (unsigned i = 0, e = HSOpts.UserEntries.size(); i != e; ++i) {
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -416,7 +416,7 @@
 HeaderSearchTriple = &PP->getAuxTargetInfo()->getTriple();
 
   ApplyHeaderSearchOptions(PP->getHeaderSearchInfo(), getHeaderSearchOpts(),
-   PP->getLangOpts(), *HeaderSearchTriple);
+   PP->getLangOpts(), PP->getDiagnostics(), 
*HeaderSearchTriple);
 
   PP->setPreprocessedOutput(getPreprocessorOutputOpts().ShowCPP);
 
Index: include/clang/Frontend/Utils.h
===
--- include/clang/Frontend/Utils.h
+++ include/clang/Frontend/Utils.h
@@ -63,6 +63,7 @@
 void ApplyHeaderSearchOptions(HeaderSearch &HS,
   const HeaderSearchOptions &HSOpts,
   const LangOptions &Lang,
+  DiagnosticsEngine &Diag,
   const llvm::Triple &triple);
 
 /// InitializePreprocessor - Initialize the preprocessor getting it and the
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -1031,3 +1031,7 @@
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
+
+// A warning group for warnings about including system headers when
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
Index: include/clang/Basic/DiagnosticCommonKinds.td
===
--- include/clang/Basic/DiagnosticCommonKinds.td
+++ include/clang/Basic/DiagnosticCommonKinds.td
@@ -248,4 +248,7 @@
 // OpenMP
 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">;
+
+// Poison system directories.
+def warn_poison_system_directories : Warning <"include location '%0' is unsafe 
for cross-compilation">, InGroup;
 }


Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderS