Include dirs pulled via pkg-config should use -isystem rather than -I cmdline switch |
|||||
Issue descriptionClang tries to be smart and avoid firing some warnings for "system" headers. This depends on distinguishing "system" headers from other headers by pulling the former via -isystem cmdline switch (rather than via -I switch). See also https://bugs.llvm.org/show_bug.cgi?id=36520 Chromium build pulls some include dirs via pkg-config - see build/config/linux/pkg_config.gni (and pkg-config.py). This groks include directories returned by the pkg-config tool (and munges them to rebase them relative to the chromium root directory). Such include directories are passed to |include_directores| key that GN understands: template("pkg_config") { ... pkgresult = exec_script(pkg_config_script, args, "value") include_dirs = pkgresult[0] cflags = pkgresult[1] The problem with the above is that it uses -I rather than -isystem flag. In particular, I've hit the following problem when trying to add -Wzero-as-null-pointer-constant to //base: $ ninja -C out/gn -j 500 -l 25 base ninja: Entering directory `out/gn' [1 + 1 / 1 @ 0.5/s : 2.093s] Regenerating ninja files [1 + 1 / 2 @ 0.9/s : 1.100s] CXX obj/base/base/message_pump_glib.o FAILED: obj/base/base/message_pump_glib.o -I../.. -Igen -I../../build/linux/debian_stretch_amd64-sysroot/usr/include/glib-2.0 ... -c ../../base/message_loop/message_pump_glib.cc -o obj/base/base/message_pump_glib.o In file included from ../../base/message_loop/message_pump_glib.cc:10: In file included from ../../build/linux/debian_stretch_amd64-sysroot/usr/include/glib-2.0/glib.h:50: In file included from ../../build/linux/debian_stretch_amd64-sysroot/usr/include/glib-2.0/glib/ghash.h:33: In file included from ../../build/linux/debian_stretch_amd64-sysroot/usr/include/glib-2.0/glib/glist.h:32: ../../build/linux/debian_stretch_amd64-sysroot/usr/include/glib-2.0/glib/gmem.h:192:10: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant] *ptr = NULL; ^~~~ nullptr ../../third_party/llvm-build/Release+Asserts/lib/clang/7.0.0/include/stddef.h:100:18: note: expanded from macro 'NULL' # define NULL __null
,
Feb 26 2018
Let pkg_config_script return -isystem instead if -I? cs.chromium.org for "isystem", I agree gn doesn't need to learn about this, we make do without it in many places.
,
Feb 26 2018
There's an old bug to do this for third_party as well: https://bugs.chromium.org/p/chromium/issues/detail?id=58751
,
Feb 26 2018
Modifying pkg-config.py seems like the the right way to go at first glance.
,
Feb 26 2018
timbrown@ ptal (feel free to unassign if you don't want this issue)
,
Feb 26 2018
The following changes seem to make this work (although I am still hitting other issues when trying to enable -Wzero-as-null-pointer-constant):
$ git diff origin/master -- build/config/linux/
diff --git a/build/config/linux/pkg-config.py b/build/config/linux/pkg-config.py
index 5ef73227df04..a3339f8e2adf 100755
--- a/build/config/linux/pkg-config.py
+++ b/build/config/linux/pkg-config.py
@@ -224,7 +224,8 @@ def main():
# Output a GN array, the first one is the cflags, the second are the libs. The
# JSON formatter prints GN compatible lists when everything is a list of
# strings.
- print json.dumps([includes, cflags, libs, lib_dirs, ldflags])
+ cflags += map(lambda dir: "-isystem" + dir, includes)
+ print json.dumps([cflags, libs, lib_dirs, ldflags])
return 0
diff --git a/build/config/linux/pkg_config.gni b/build/config/linux/pkg_config.gni
index 7358f8e763c7..8f95457e928f 100644
--- a/build/config/linux/pkg_config.gni
+++ b/build/config/linux/pkg_config.gni
@@ -101,13 +101,12 @@ template("pkg_config") {
}
pkgresult = exec_script(pkg_config_script, args, "value")
- include_dirs = pkgresult[0]
- cflags = pkgresult[1]
+ cflags = pkgresult[0]
if (!defined(invoker.ignore_libs) || !invoker.ignore_libs) {
- libs = pkgresult[2]
- lib_dirs = pkgresult[3]
- ldflags = pkgresult[4]
+ libs = pkgresult[1]
+ lib_dirs = pkgresult[2]
+ ldflags = pkgresult[3]
}
Feel free to wrap the above in the CL and land it (I'll try to push on -Wzero-as-null-pointer-constant a bit more to see what else might block this warning [cough... gtest... cough... :o])
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bfd2d6258525de1dbaff7767fd3b8c0b25bb96ff commit bfd2d6258525de1dbaff7767fd3b8c0b25bb96ff Author: Tim Brown <timbrown@chromium.org> Date: Tue Feb 27 18:17:08 2018 Make includes from pkg-config use -isystem not -I pkg-config currently specifies include directories using the -I flag, even for system libraries. This means that warnings in the included header files (for example when enabling -Wzero-as-null-pointer-constant) cause the build to fail. This change will make pkg-config include directories included using the -isystem flag instead, which will make the compiler not report warnings in the included header files. Bug: 816565 Change-Id: I441ddeee28f8031410ce95ae1712bd68b0b86a63 Reviewed-on: https://chromium-review.googlesource.com/938941 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Commit-Queue: Tim Brown <timbrown@chromium.org> Cr-Commit-Position: refs/heads/master@{#539500} [modify] https://crrev.com/bfd2d6258525de1dbaff7767fd3b8c0b25bb96ff/build/config/linux/pkg-config.py [modify] https://crrev.com/bfd2d6258525de1dbaff7767fd3b8c0b25bb96ff/build/config/linux/pkg_config.gni
,
Feb 27 2018
Thanks for the suggested code change. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by lukasza@chromium.org
, Feb 26 2018