Check for system include paths by looking at clang's HeaderSearchOptions,

Check for system include paths by looking at clang's HeaderSearchOptions,
rather than assuming all absolute paths are system include paths.

Add a test for relative --sysroots

BUG=

Committed: https://crrev.com/b9ea19b29ec99621d3eb77b90d51ea7af6d0b9ef
Cr-Commit-Position: refs/heads/master@{#373184}

[dsansome, 2016-02-01 08:23:46]

dsansome@chromium.org changed reviewers:
+ akuegel@chromium.org, zforman@google.com

[Adrian Kuegel, 2016-02-01 09:46:25]

Could you please also test this on real data? If everything works correctly, the
index pack data for the same revision of Chrome should not change. Probably it
is also sufficient to just look at some of the .filepaths files that this tool
generates and compare them.

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:89: for (const auto&
prefix : header_search_options->SystemHeaderPrefixes) {
In the documentation this is saying "User-specified system header prefixes".
Does that mean it only includes the user specified system header prefixes (i.e.
all those -isystem flags)? Or are there any defaults? I think in the compile
commands for Chrome we don't explicitly add -isystem flags for standard system
headers.

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:93: for (const auto&
entry : header_search_options->UserEntries) {
I am not so sure about if this works correctly. If I understand this code, this
deals with libraries that are specified with the -I option. In case of Chrome
that means headers within the Chrome source, and they are specified with
relative path in the -I option. For cross references to work correctly we need
to have an exact match between the path Grok knows, and the path Codesearch
knows. So right now for all headers inside Chrome we find them by relative path,
and that is working fine.

[Adrian Kuegel, 2016-02-01 09:54:10]

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:89: for (const auto&
prefix : header_search_options->SystemHeaderPrefixes) {
On 2016/02/01 09:46:25, Adrian Kuegel wrote:
> In the documentation this is saying "User-specified system header prefixes".
> Does that mean it only includes the user specified system header prefixes
(i.e.
> all those -isystem flags)? Or are there any defaults? I think in the compile
> commands for Chrome we don't explicitly add -isystem flags for standard system
> headers.

I just asked a team mate, and he told me that it includes also the standard
system headers. So this part is probably fine.

[dsansome, 2016-02-02 06:48:59]

> Could you please also test this on real data? If everything works correctly,
the
> index pack data for the same revision of Chrome should not change. Probably it
> is also sufficient to just look at some of the .filepaths files that this tool
> generates and compare them.

I ran this over Chromium and compared the diffs before and after my change.

The .filepaths files are identical except for some system includes that are now
correctly detected as being in a system include path and have a //.  The diff is
5854687 lines long but here's an example (I've replaced $HOME with "[snip]"):

 ../../ui/views/widget/widget.h
 ../../ui/views/window/client_view.h
 ../../ui/views/window/non_client_view.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/alloca.h
+[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//alloca.h

[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//asm-generic/errno-base.h

[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//asm-generic/errno.h
 [snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//assert.h
 [snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//ctype.h
 [snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//endian.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/endian.h
 [snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//errno.h
 [snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//features.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/features.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/_G_config.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/getopt.h
-[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/libio.h
+[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//_G_config.h
+[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//getopt.h
+[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include//libio.h

Replacing // with / with sed and sorting the outputs gives no diff:
for f in $(find -name '*.filepaths'); do diff -u <(sed 's|//|/|' $f | sort -u)
<(sed 's|//|/|' ../filepaths-new/$f | sort -u); done  | less

I compared the index packs too.  The old index packs had some duplicate entries
in the required_input section (probably because the .filepaths had one with //
and one with /, and it canonicalised them somewhere along the way):

diff -u <(zcat pack_old_net_util/units/* | jq .content.required_input[].path |
sort) <(zcat pack_new_net_util/units/* | jq .content.required_input[].path |
sort)


"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/char_traits.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/concept_check.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/cpp_type_traits.h"
-"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/cpp_type_traits.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/cxxabi_forced.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/exception_defines.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/exception_ptr.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/functexcept.h"
-"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/functexcept.h"
-"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/functional_hash.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/functional_hash.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/hash_bytes.h"
-"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/hash_bytes.h"

"[snip]src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/hashtable.h"

I wrote a dumb python script to canonicalise the unit files (remove duplicate
required_inputs and sort the list), and it shows no diffs!

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:89: for (const auto&
prefix : header_search_options->SystemHeaderPrefixes) {
On 2016/02/01 09:54:10, Adrian Kuegel wrote:
> On 2016/02/01 09:46:25, Adrian Kuegel wrote:
> > In the documentation this is saying "User-specified system header prefixes".
> > Does that mean it only includes the user specified system header prefixes
> (i.e.
> > all those -isystem flags)? Or are there any defaults? I think in the compile
> > commands for Chrome we don't explicitly add -isystem flags for standard
system
> > headers.
> 
> I just asked a team mate, and he told me that it includes also the standard
> system headers. So this part is probably fine.

It seems to be empty for me anyway.  All the system headers are in the other
list in the System and ExternCSystem groups.  *shrug*

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:93: for (const auto&
entry : header_search_options->UserEntries) {
On 2016/02/01 09:46:25, Adrian Kuegel wrote:
> I am not so sure about if this works correctly. If I understand this code,
this
> deals with libraries that are specified with the -I option. In case of Chrome
> that means headers within the Chrome source, and they are specified with
> relative path in the -I option. For cross references to work correctly we need
> to have an exact match between the path Grok knows, and the path Codesearch
> knows. So right now for all headers inside Chrome we find them by relative
path,
> and that is working fine.

This list has all the paths in, but ones from -I have Group
clang::frontend::Angled which I don't insert into system_header_prefixes_ - see
the comments for IncludeDirGroup in
llvm/tools/clang/include/clang/Lex/HeaderSearchOptions.h.

I ran this and printf'd all the entries.  SystemHeaderPrefixes contained
nothing, UserEntries contained the following:

clang::frontend::Angled:
../..
gen
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/gconf/2
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/dbus-1.0
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
../../third_party/protobuf/src
gen/protoc_out
../../third_party/protobuf/src
../../third_party/protobuf
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/nss
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/nspr
../../third_party/boringssl/src/include
../../sdch/open-vcdiff/src
../../third_party/zlib
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
../../third_party/icu/source/common
../../third_party/icu/source/i18n

clang::frontend::System:
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/backward
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/local/include
[snip]/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.9.0/include

clang::frontend::ExternCSystem:
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/include
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/include

I've added a comment here to explain what's going on.

[Adrian Kuegel, 2016-02-02 08:37:19]

LGTM
Thanks for verifying that everything should still work. Also great that we can
get rid of duplicate headers. Great work :)

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:89: for (const auto&
prefix : header_search_options->SystemHeaderPrefixes) {
On 2016/02/02 06:48:58, dsansome wrote:
> On 2016/02/01 09:54:10, Adrian Kuegel wrote:
> > On 2016/02/01 09:46:25, Adrian Kuegel wrote:
> > > In the documentation this is saying "User-specified system header
prefixes".
> > > Does that mean it only includes the user specified system header prefixes
> > (i.e.
> > > all those -isystem flags)? Or are there any defaults? I think in the
compile
> > > commands for Chrome we don't explicitly add -isystem flags for standard
> system
> > > headers.
> > 
> > I just asked a team mate, and he told me that it includes also the standard
> > system headers. So this part is probably fine.
> 
> It seems to be empty for me anyway.  All the system headers are in the other
> list in the System and ExternCSystem groups.  *shrug*

Maybe I misunderstood him, or he was wrong. Anyway, good to know that the
standard header prefixes are there.

https://codereview.chromium.org/1658553002/diff/1/tools/clang/translation_uni...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:93: for (const auto&
entry : header_search_options->UserEntries) {
On 2016/02/02 06:48:58, dsansome wrote:
> On 2016/02/01 09:46:25, Adrian Kuegel wrote:
> > I am not so sure about if this works correctly. If I understand this code,
> this
> > deals with libraries that are specified with the -I option. In case of
Chrome
> > that means headers within the Chrome source, and they are specified with
> > relative path in the -I option. For cross references to work correctly we
need
> > to have an exact match between the path Grok knows, and the path Codesearch
> > knows. So right now for all headers inside Chrome we find them by relative
> path,
> > and that is working fine.
> 
> This list has all the paths in, but ones from -I have Group
> clang::frontend::Angled which I don't insert into system_header_prefixes_ -
see
> the comments for IncludeDirGroup in
> llvm/tools/clang/include/clang/Lex/HeaderSearchOptions.h.
> 
> I ran this and printf'd all the entries.  SystemHeaderPrefixes contained
> nothing, UserEntries contained the following:
> 
> clang::frontend::Angled:
> ../..
> gen
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/gconf/2
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/dbus-1.0
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
> ../../third_party/protobuf/src
> gen/protoc_out
> ../../third_party/protobuf/src
> ../../third_party/protobuf
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/nss
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/nspr
> ../../third_party/boringssl/src/include
> ../../sdch/open-vcdiff/src
> ../../third_party/zlib
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
> ../../third_party/icu/source/common
> ../../third_party/icu/source/i18n
> 
> clang::frontend::System:
>
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6
>
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu
>
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/backward
> [snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/local/include
>
[snip]/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.9.0/include
> 
> clang::frontend::ExternCSystem:
>
[snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu
> [snip]/src/build/linux/debian_wheezy_amd64-sysroot/include
> [snip]/src/build/linux/debian_wheezy_amd64-sysroot/usr/include
> 
> I've added a comment here to explain what's going on.

Interesting; that seems to indicate that Chromium builds using a special sysroot
in src/build. I didn't know that, either this has changed recently, or I never
noticed that the absolute paths were pointing to within the Chrome source tree.
Anyway, looks like everything is fine.

[dsansome, 2016-02-02 23:26:35]

On 2016/02/02 08:37:19, Adrian Kuegel wrote:
> Interesting; that seems to indicate that Chromium builds using a special
sysroot
> in src/build. I didn't know that, either this has changed recently, or I never
> noticed that the absolute paths were pointing to within the Chrome source
tree.
> Anyway, looks like everything is fine.

Yeah looks like it's been the default for Clang on Linux since November:
https://codereview.chromium.org/1472873004

I'll land this now and get ready to revert it when it inevitably breaks
codesearch!  Thanks for the review!

[dsansome, 2016-02-02 23:26:41]

The CQ bit was checked by dsansome@chromium.org

[commit-bot: I haz the power, 2016-02-02 23:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658553002/40001

[commit-bot: I haz the power, 2016-02-02 23:51:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-02 23:51:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dsansome, 2016-02-03 00:15:47]

dsansome@chromium.org changed reviewers:
+ dcheng@chromium.org

[dsansome, 2016-02-03 00:15:48]

+dcheng for tools/clang OWNERS

[dcheng, 2016-02-03 05:18:33]

lgtm

https://codereview.chromium.org/1658553002/diff/40001/tools/clang/translation...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/1658553002/diff/40001/tools/clang/translation...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:252: for (const
string& path : source_file_paths_) {
Oops.

[dsansome, 2016-02-03 05:35:58]

The CQ bit was checked by dsansome@chromium.org

[commit-bot: I haz the power, 2016-02-03 05:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658553002/40001

[commit-bot: I haz the power, 2016-02-03 05:41:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-03 05:42:44]

Description was changed from

==========
Check for system include paths by looking at clang's HeaderSearchOptions,
rather than assuming all absolute paths are system include paths.

Add a test for relative --sysroots

BUG=
==========

to

==========
Check for system include paths by looking at clang's HeaderSearchOptions,
rather than assuming all absolute paths are system include paths.

Add a test for relative --sysroots

BUG=

Committed: https://crrev.com/b9ea19b29ec99621d3eb77b90d51ea7af6d0b9ef
Cr-Commit-Position: refs/heads/master@{#373184}
==========

[commit-bot: I haz the power, 2016-02-03 05:42:45]

Patchset 3 (id:??) landed as
https://crrev.com/b9ea19b29ec99621d3eb77b90d51ea7af6d0b9ef
Cr-Commit-Position: refs/heads/master@{#373184}