Add a clang tool to generate lists of file paths involved in a compilation.

Add a clang tool to generate lists of file paths involved in a compilation.

This tool is used to support Codesearch for Chromium. For each compilation
unit, a file ending in <main source file>.filepath is produced which
contains a list of all files involved in the compilation.

The files produced by the tool will be processed by a script (TBD) that copies
all files mentioned as a dependency in one of these files to a common
folder (first renamed based on SHA256). Also, for each compilation unit it
creates a file containing the compilation arguments and references to the
renamed files needed for the compilation.

BUG=429684
Committed: https://crrev.com/9ab5616881e04d5f790ce94052ccc4b9036ef836
Cr-Commit-Position: refs/heads/master@{#304801}

[Adrian Kuegel, 2014-11-03 16:08:14]

akuegel@chromium.org changed reviewers:
+ thakis@chromium.org

[Adrian Kuegel, 2014-11-03 16:08:15]

Nico, can you please review this CL?
This is just the first step, I haven't yet finished the script that will process
these .filepaths files. I plan to do that in a separate CL.

[Nico, 2014-11-03 18:12:54]

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

[Nico, 2014-11-03 18:12:55]

So the plan is that the codesearch bot always has an llvm checkout? And it'll
build this tool as a build step?

Code looks fine, maybe you can add a test?

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:50: FileChangeReason
reason,
clang::FileChangeReason (not really needed because base class is in clang, but
nicer anyhow)

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:78: FileChangeReason
reason,
clang::

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:81: if (reason ==
EnterFile) {
clang::

[Adrian Kuegel, 2014-11-03 19:00:04]

On 2014/11/03 18:12:55, Nico wrote:
> So the plan is that the codesearch bot always has an llvm checkout? And it'll
> build this tool as a build step?

Yes. I was under the impression that a regular Chrome checkout already includes
the llvm checkout. Is that correct? If yes, I think it is relatively easy to add
the build step to the Codesearch recipe.

> Code looks fine, maybe you can add a test?

I will think about how I can best do that. The output to files does not work
nice with the existing test tool.

>
https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
> File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):
> 
>
https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
> tools/clang/translation_unit/TranslationUnitGenerator.cpp:50: FileChangeReason
> reason,
> clang::FileChangeReason (not really needed because base class is in clang, but
> nicer anyhow)
> 
>
https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
> tools/clang/translation_unit/TranslationUnitGenerator.cpp:78: FileChangeReason
> reason,
> clang::
> 
>
https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
> tools/clang/translation_unit/TranslationUnitGenerator.cpp:81: if (reason ==
> EnterFile) {
> clang::

[Nico, 2014-11-03 19:02:28]

On Mon, Nov 3, 2014 at 11:00 AM, <akuegel@chromium.org> wrote:

> On 2014/11/03 18:12:55, Nico wrote:
>
>> So the plan is that the codesearch bot always has an llvm checkout? And
>> it'll
>> build this tool as a build step?
>>
>
> Yes. I was under the impression that a regular Chrome checkout already
> includes
> the llvm checkout. Is that correct?


No, that's not correct. Regular chrome checkouts download prebuilt clang
binaries and don't include llvm checkouts.


> If yes, I think it is relatively easy to add
> the build step to the Codesearch recipe.
>
>  Code looks fine, maybe you can add a test?
>>
>
> I will think about how I can best do that. The output to files does not
> work
> nice with the existing test tool.
>
>
>
> https://codereview.chromium.org/696303002/diff/20001/
> tools/clang/translation_unit/TranslationUnitGenerator.cpp
>
>> File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):
>>
>
>
> https://codereview.chromium.org/696303002/diff/20001/
> tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode50
>
>> tools/clang/translation_unit/TranslationUnitGenerator.cpp:50:
>> FileChangeReason
>> reason,
>> clang::FileChangeReason (not really needed because base class is in
>> clang, but
>> nicer anyhow)
>>
>
>
> https://codereview.chromium.org/696303002/diff/20001/
> tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode78
>
>> tools/clang/translation_unit/TranslationUnitGenerator.cpp:78:
>> FileChangeReason
>> reason,
>> clang::
>>
>
>
> https://codereview.chromium.org/696303002/diff/20001/
> tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode81
>
>> tools/clang/translation_unit/TranslationUnitGenerator.cpp:81: if (reason
>> ==
>> EnterFile) {
>> clang::
>>
>
>
>
> https://codereview.chromium.org/696303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Adrian Kuegel, 2014-11-03 19:17:35]

On 2014/11/03 19:02:28, Nico wrote:
> On Mon, Nov 3, 2014 at 11:00 AM, <mailto:akuegel@chromium.org> wrote:
> 
> > On 2014/11/03 18:12:55, Nico wrote:
> >
> >> So the plan is that the codesearch bot always has an llvm checkout? And
> >> it'll
> >> build this tool as a build step?
> >>
> >
> > Yes. I was under the impression that a regular Chrome checkout already
> > includes
> > the llvm checkout. Is that correct?
> 
> 
> No, that's not correct. Regular chrome checkouts download prebuilt clang
> binaries and don't include llvm checkouts.

Ok, I see. But the script tools/clang/scripts/update.sh seems to do that, so it
still should be easy to add to the codesearch recipe.

> 
> > If yes, I think it is relatively easy to add
> > the build step to the Codesearch recipe.
> >
> >  Code looks fine, maybe you can add a test?
> >>
> >
> > I will think about how I can best do that. The output to files does not
> > work
> > nice with the existing test tool.
> >
> >
> >
> > https://codereview.chromium.org/696303002/diff/20001/
> > tools/clang/translation_unit/TranslationUnitGenerator.cpp
> >
> >> File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):
> >>
> >
> >
> > https://codereview.chromium.org/696303002/diff/20001/
> > tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode50
> >
> >> tools/clang/translation_unit/TranslationUnitGenerator.cpp:50:
> >> FileChangeReason
> >> reason,
> >> clang::FileChangeReason (not really needed because base class is in
> >> clang, but
> >> nicer anyhow)
> >>
> >
> >
> > https://codereview.chromium.org/696303002/diff/20001/
> > tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode78
> >
> >> tools/clang/translation_unit/TranslationUnitGenerator.cpp:78:
> >> FileChangeReason
> >> reason,
> >> clang::
> >>
> >
> >
> > https://codereview.chromium.org/696303002/diff/20001/
> > tools/clang/translation_unit/TranslationUnitGenerator.cpp#newcode81
> >
> >> tools/clang/translation_unit/TranslationUnitGenerator.cpp:81: if (reason
> >> ==
> >> EnterFile) {
> >> clang::
> >>
> >
> >
> >
> > https://codereview.chromium.org/696303002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Adrian Kuegel, 2014-11-07 14:44:11]

Sorry for the delay. I was Chromium sheriff Tuesday and Wednesday, and Thursday
I was OOO.

Today I have tried adding a test, but ran into some issues.
I was thinking of adding a simple compilation unit called test.cc that is
essentially a hello world program, and then put a test.cc.filepaths.expected
file into the test folder. But that doesn't really work, because even such a
simple program involves some clang headers (stdarg.h and stddef.h), which have
paths relative to the location of the clang executable. So this path would be
different on all systems. So a diff tool would have to exclude all clang
headers, but even then, some of the other included file headers are also OS
specific. For example /usr/include/x86_64-linux-gnu/bits/types.h. And I also
have the fear that this sort of test might break easily, even if the tool itself
would still work correctly.

Do you have an idea how to avoid these issues, and still have a unit test and
not an integration test?

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
File tools/clang/translation_unit/TranslationUnitGenerator.cpp (right):

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:50: FileChangeReason
reason,
On 2014/11/03 18:12:54, Nico wrote:
> clang::FileChangeReason (not really needed because base class is in clang, but
> nicer anyhow)

Done.

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:78: FileChangeReason
reason,
On 2014/11/03 18:12:54, Nico wrote:
> clang::

Done.

https://codereview.chromium.org/696303002/diff/20001/tools/clang/translation_...
tools/clang/translation_unit/TranslationUnitGenerator.cpp:81: if (reason ==
EnterFile) {
On 2014/11/03 18:12:54, Nico wrote:
> clang::

Done.

[Nico, 2014-11-07 18:11:09]

On Fri, Nov 7, 2014 at 6:44 AM, <akuegel@chromium.org> wrote:

> Sorry for the delay. I was Chromium sheriff Tuesday and Wednesday, and
> Thursday
> I was OOO.
>
> Today I have tried adding a test, but ran into some issues.
> I was thinking of adding a simple compilation unit called test.cc that is
> essentially a hello world program, and then put a
> test.cc.filepaths.expected
> file into the test folder. But that doesn't really work, because even such
> a
> simple program involves some clang headers (stdarg.h and stddef.h), which
> have
> paths relative to the location of the clang executable. So this path would
> be
> different on all systems. So a diff tool would have to exclude all clang
> headers, but even then, some of the other included file headers are also OS
> specific. For example /usr/include/x86_64-linux-gnu/bits/types.h. And I
> also
> have the fear that this sort of test might break easily, even if the tool
> itself
> would still work correctly.
>
> Do you have an idea how to avoid these issues, and still have a unit test
> and
> not an integration test?


Don't use system headers, only your own .h files?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Adrian Kuegel, 2014-11-12 16:21:54]

Patchset #4 (id:60001) has been deleted

[Adrian Kuegel, 2014-11-12 18:00:24]

Hi Nico,

I added a test script and some test files. In the end, even if I avoid system
headers, the paths the tool generates are still absolute paths, so my tool now
only compares the filenames and check if they match.
The test tool is based on the test_tool.py script. Maybe test_tool.py could be
rewritten to handle my case as well, but it might not be worth it if there are
no other cases that need a test like my tool.

[Adrian Kuegel, 2014-11-17 12:50:44]

On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> Hi Nico,
> 
> I added a test script and some test files. In the end, even if I avoid system
> headers, the paths the tool generates are still absolute paths, so my tool now
> only compares the filenames and check if they match.
> The test tool is based on the test_tool.py script. Maybe test_tool.py could be
> rewritten to handle my case as well, but it might not be worth it if there are
> no other cases that need a test like my tool.

Nico, can you please take another look? I will be OOO tomorrow (Tuesday), so it
is not urgent, but it would be nice if you could take a look in the next two
days. Thanks :)

@Daniel: could you please take a look at test_translation_unit.py which is based
on your test_tool.py and tell me if you agree with that copy, or if I should try
to include the necessary logic into test_tool.py directly.

[dcheng, 2014-11-17 18:34:29]

On 2014/11/17 at 12:50:44, akuegel wrote:
> On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> > Hi Nico,
> > 
> > I added a test script and some test files. In the end, even if I avoid
system
> > headers, the paths the tool generates are still absolute paths, so my tool
now
> > only compares the filenames and check if they match.
> > The test tool is based on the test_tool.py script. Maybe test_tool.py could
be
> > rewritten to handle my case as well, but it might not be worth it if there
are
> > no other cases that need a test like my tool.
> 
> Nico, can you please take another look? I will be OOO tomorrow (Tuesday), so
it is not urgent, but it would be nice if you could take a look in the next two
days. Thanks :)
> 
> @Daniel: could you please take a look at test_translation_unit.py which is
based on your test_tool.py and tell me if you agree with that copy, or if I
should try to include the necessary logic into test_tool.py directly.

Do you ever intend for this tool to work on Windows in the future? If so, it
might be nice to share as much as possible with test_tool.py, since I'm (sort
of) working on making things work there.

Another thought: could the tool make paths relative to the chrome src root when
possible? That might eliminate the special logic you needed to add for paths.

[Nico, 2014-11-17 23:51:55]

lgtm

Sharing things like dcheng suggested sounds like a good idea going forward, but
I think this is in a good shape to go in.

[Adrian Kuegel, 2014-11-19 12:44:49]

On 2014/11/17 18:34:29, dcheng wrote:
> On 2014/11/17 at 12:50:44, akuegel wrote:
> > On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> > > Hi Nico,
> > > 
> > > I added a test script and some test files. In the end, even if I avoid
> system
> > > headers, the paths the tool generates are still absolute paths, so my tool
> now
> > > only compares the filenames and check if they match.
> > > The test tool is based on the test_tool.py script. Maybe test_tool.py
could
> be
> > > rewritten to handle my case as well, but it might not be worth it if there
> are
> > > no other cases that need a test like my tool.
> > 
> > Nico, can you please take another look? I will be OOO tomorrow (Tuesday), so
> it is not urgent, but it would be nice if you could take a look in the next
two
> days. Thanks :)
> > 
> > @Daniel: could you please take a look at test_translation_unit.py which is
> based on your test_tool.py and tell me if you agree with that copy, or if I
> should try to include the necessary logic into test_tool.py directly.
> 
> Do you ever intend for this tool to work on Windows in the future? If so, it
> might be nice to share as much as possible with test_tool.py, since I'm (sort
> of) working on making things work there.
> 
> Another thought: could the tool make paths relative to the chrome src root
when
> possible? That might eliminate the special logic you needed to add for paths.

Yes, if possible I want to make this tool work on Windows in the future. First
step would be Linux and Mac OS, however.
Nice to know that you work on Windows support. I will keep a look at the changes
to test_tool.py and try to integrate them.
About the paths: I guess just using paths relative to src/tools/clang should be
even easier, because all relevant directories are subdirectories of that. I
changed that now.

[Adrian Kuegel, 2014-11-19 12:45:54]

The CQ bit was checked by akuegel@chromium.org

[commit-bot: I haz the power, 2014-11-19 12:46:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/696303002/120001

[commit-bot: I haz the power, 2014-11-19 12:50:26]

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

[commit-bot: I haz the power, 2014-11-19 12:50:27]

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

[Adrian Kuegel, 2014-11-19 12:53:06]

The CQ bit was unchecked by akuegel@chromium.org

[Adrian Kuegel, 2014-11-19 13:09:59]

The CQ bit was checked by akuegel@chromium.org

[commit-bot: I haz the power, 2014-11-19 13:11:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/696303002/160001

[commit-bot: I haz the power, 2014-11-19 14:00:19]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2014-11-19 14:01:06]

Patchset 8 (id:??) landed as
https://crrev.com/9ab5616881e04d5f790ce94052ccc4b9036ef836
Cr-Commit-Position: refs/heads/master@{#304801}

[dcheng, 2014-11-19 16:01:58]

On 2014/11/19 at 12:44:49, akuegel wrote:
> On 2014/11/17 18:34:29, dcheng wrote:
> > On 2014/11/17 at 12:50:44, akuegel wrote:
> > > On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> > > > Hi Nico,
> > > > 
> > > > I added a test script and some test files. In the end, even if I avoid
> > system
> > > > headers, the paths the tool generates are still absolute paths, so my
tool
> > now
> > > > only compares the filenames and check if they match.
> > > > The test tool is based on the test_tool.py script. Maybe test_tool.py
could
> > be
> > > > rewritten to handle my case as well, but it might not be worth it if
there
> > are
> > > > no other cases that need a test like my tool.
> > > 
> > > Nico, can you please take another look? I will be OOO tomorrow (Tuesday),
so
> > it is not urgent, but it would be nice if you could take a look in the next
two
> > days. Thanks :)
> > > 
> > > @Daniel: could you please take a look at test_translation_unit.py which is
> > based on your test_tool.py and tell me if you agree with that copy, or if I
> > should try to include the necessary logic into test_tool.py directly.
> > 
> > Do you ever intend for this tool to work on Windows in the future? If so, it
> > might be nice to share as much as possible with test_tool.py, since I'm
(sort
> > of) working on making things work there.
> > 
> > Another thought: could the tool make paths relative to the chrome src root
when
> > possible? That might eliminate the special logic you needed to add for
paths.
> 
> Yes, if possible I want to make this tool work on Windows in the future. First
step would be Linux and Mac OS, however.
> Nice to know that you work on Windows support. I will keep a look at the
changes to test_tool.py and try to integrate them.
> About the paths: I guess just using paths relative to src/tools/clang should
be even easier, because all relevant directories are subdirectories of that. I
changed that now.

FWIW, I was hoping that making things relative would allow us to simplify/not
need test_translation_unit.py. =)

[Adrian Kuegel, 2014-11-19 16:06:44]

On 2014/11/19 16:01:58, dcheng wrote:
> On 2014/11/19 at 12:44:49, akuegel wrote:
> > On 2014/11/17 18:34:29, dcheng wrote:
> > > On 2014/11/17 at 12:50:44, akuegel wrote:
> > > > On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> > > > > Hi Nico,
> > > > > 
> > > > > I added a test script and some test files. In the end, even if I avoid
> > > system
> > > > > headers, the paths the tool generates are still absolute paths, so my
> tool
> > > now
> > > > > only compares the filenames and check if they match.
> > > > > The test tool is based on the test_tool.py script. Maybe test_tool.py
> could
> > > be
> > > > > rewritten to handle my case as well, but it might not be worth it if
> there
> > > are
> > > > > no other cases that need a test like my tool.
> > > > 
> > > > Nico, can you please take another look? I will be OOO tomorrow
(Tuesday),
> so
> > > it is not urgent, but it would be nice if you could take a look in the
next
> two
> > > days. Thanks :)
> > > > 
> > > > @Daniel: could you please take a look at test_translation_unit.py which
is
> > > based on your test_tool.py and tell me if you agree with that copy, or if
I
> > > should try to include the necessary logic into test_tool.py directly.
> > > 
> > > Do you ever intend for this tool to work on Windows in the future? If so,
it
> > > might be nice to share as much as possible with test_tool.py, since I'm
> (sort
> > > of) working on making things work there.
> > > 
> > > Another thought: could the tool make paths relative to the chrome src root
> when
> > > possible? That might eliminate the special logic you needed to add for
> paths.
> > 
> > Yes, if possible I want to make this tool work on Windows in the future.
First
> step would be Linux and Mac OS, however.
> > Nice to know that you work on Windows support. I will keep a look at the
> changes to test_tool.py and try to integrate them.
> > About the paths: I guess just using paths relative to src/tools/clang should
> be even easier, because all relevant directories are subdirectories of that. I
> changed that now.
> 
> FWIW, I was hoping that making things relative would allow us to simplify/not
> need test_translation_unit.py. =)

Sorry, seems I misunderstood you. Yes, we can probably avoid it, but at the cost
of adding more logic to test_tool.py which is only needed for my tool (like the
special diff logic). Maybe a better way would be if I could include common code
into my test script?

[dcheng, 2014-11-19 16:11:59]

On 2014/11/19 at 16:06:44, akuegel wrote:
> On 2014/11/19 16:01:58, dcheng wrote:
> > On 2014/11/19 at 12:44:49, akuegel wrote:
> > > On 2014/11/17 18:34:29, dcheng wrote:
> > > > On 2014/11/17 at 12:50:44, akuegel wrote:
> > > > > On 2014/11/12 18:00:24, Adrian Kuegel wrote:
> > > > > > Hi Nico,
> > > > > > 
> > > > > > I added a test script and some test files. In the end, even if I
avoid
> > > > system
> > > > > > headers, the paths the tool generates are still absolute paths, so
my
> > tool
> > > > now
> > > > > > only compares the filenames and check if they match.
> > > > > > The test tool is based on the test_tool.py script. Maybe
test_tool.py
> > could
> > > > be
> > > > > > rewritten to handle my case as well, but it might not be worth it if
> > there
> > > > are
> > > > > > no other cases that need a test like my tool.
> > > > > 
> > > > > Nico, can you please take another look? I will be OOO tomorrow
(Tuesday),
> > so
> > > > it is not urgent, but it would be nice if you could take a look in the
next
> > two
> > > > days. Thanks :)
> > > > > 
> > > > > @Daniel: could you please take a look at test_translation_unit.py
which is
> > > > based on your test_tool.py and tell me if you agree with that copy, or
if I
> > > > should try to include the necessary logic into test_tool.py directly.
> > > > 
> > > > Do you ever intend for this tool to work on Windows in the future? If
so, it
> > > > might be nice to share as much as possible with test_tool.py, since I'm
> > (sort
> > > > of) working on making things work there.
> > > > 
> > > > Another thought: could the tool make paths relative to the chrome src
root
> > when
> > > > possible? That might eliminate the special logic you needed to add for
> > paths.
> > > 
> > > Yes, if possible I want to make this tool work on Windows in the future.
First
> > step would be Linux and Mac OS, however.
> > > Nice to know that you work on Windows support. I will keep a look at the
> > changes to test_tool.py and try to integrate them.
> > > About the paths: I guess just using paths relative to src/tools/clang
should
> > be even easier, because all relevant directories are subdirectories of that.
I
> > changed that now.
> > 
> > FWIW, I was hoping that making things relative would allow us to
simplify/not
> > need test_translation_unit.py. =)
> 
> Sorry, seems I misunderstood you. Yes, we can probably avoid it, but at the
cost of adding more logic to test_tool.py which is only needed for my tool (like
the special diff logic). Maybe a better way would be if I could include common
code into my test script?

I guess this way is fine. If we can simplify in the future, that would be nice,
but no big deal now.