This is an archive of the discontinued LLVM Phabricator instance.

[clang-cl] Print /showIncludes to stderr, if used in combination with /E, /EP or /P
ClosedPublic

Authored by fxb on May 3 2018, 8:28 AM.

Details

Summary

This replicates 'cl.exe' behavior and allows for both preprocessor output and
dependency information to be extraced with a single compiler invocation.

This is especially useful for compiler caching with tools like Mozilla's sccache.

See: https://github.com/mozilla/sccache/issues/246

Diff Detail

Event Timeline

fxb created this revision.May 3 2018, 8:28 AM
fxb added a comment.May 3 2018, 8:33 AM

This is my first patch to clang, so any feedback regarding implementation appreciated!

Also, let me know if you have any suggestions on how to add more extensive tests for this.

erichkeane added inline comments.May 3 2018, 8:41 AM
include/clang/Frontend/DependencyOutputOptions.h
33–34

Doing these two as separate options is a touch annoying. First, there is an extra state that ends up being possible but ignored. I'd prefer making a "PrintShowIncludeDestination : 2" (name open for proper bikeshed) that contains an enum so that the states are explicit. Something like:
None,
StdOut,
StdErr

That way, the 4th state is explicitly unused.

test/Driver/cl-options.c
203

I'm perhaps missing something here... why did "/EP /P /showIncludes" previously NOT warn?

fxb added inline comments.May 3 2018, 8:49 AM
include/clang/Frontend/DependencyOutputOptions.h
33–34

Yes, that definitely makes sense and is a lot nicer than having these two options, which I found quite ugly as well.

I will fix that and upload a new patch set.

test/Driver/cl-options.c
203

/P will preprocess to a file, so it was compatible with /showIncludes, which was printed to stdout.

With this change all preprocessor options are now possible to use in combination with /showIncludes, since its output will be written to stderr, like cl.exe does.

I was actually not sure if I shall keep these tests at all after my change or just invert them, like I ended up doing here. There is probably a better way to actually test that /showIncludes output will end up on stderr, but with my limited experience I didn't figure that out yet. Let me know if you have any ideas how to improve these tests.

fxb updated this revision to Diff 145030.May 3 2018, 9:19 AM

Updated the code to use an enum called ShowIncludesDestination

fxb marked 2 inline comments as done.May 3 2018, 9:20 AM
erichkeane accepted this revision.May 3 2018, 9:22 AM

This looks fine to me. Let me know if you need me to commit this for you.

This revision is now accepted and ready to land.May 3 2018, 9:22 AM
fxb added a comment.May 3 2018, 9:30 AM

@erichkeane That would be great! Thanks!

erichkeane requested changes to this revision.May 3 2018, 10:18 AM

When building this, it didn't build since you'd reused the name. I changed the variable to be named "Dest" instead of "Destination"

That said, the test suite now fails on Clang :: Frontend/print-header-includes.c'.

Please do a build with 'check-clang' to see why this no longer works.
Thanks!
-Erich

This revision now requires changes to proceed.May 3 2018, 10:18 AM
fxb added a comment.May 3 2018, 10:38 AM

I'll have a look at these issues tomorrow and submit a new patch then.

Thanks for all your help so far!

fxb updated this revision to Diff 145196.May 4 2018, 8:10 AM
  1. Fixed the compile error caused by re-using the name ShowIncludesDestination. The member variable is now named ShowIncludesDest.
  2. Fixed test/Frontend/print-header-includes.c to test both cases:
    • If only --show-includes is passed, includes are printed on stdout.
    • If both --show-includes and -E are passed, includes are printed on stderr.
erichkeane accepted this revision.May 4 2018, 9:01 AM
This revision is now accepted and ready to land.May 4 2018, 9:01 AM
This revision was automatically updated to reflect the committed changes.