This is an archive of the discontinued LLVM Phabricator instance.

Fix for bug 38508 - Don't do PCH processing when only generating preprocessor output
ClosedPublic

Authored by mikerice on Aug 13 2018, 8:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mikerice created this revision.Aug 13 2018, 8:59 AM
thakis accepted this revision.Aug 15 2018, 12:37 PM

The approach lgtm, thanks.

How does the gcc driver codepath handle this? Does it just not have to worry about this because it doesn't have something like warn_pp_macro_def_mismatch_with_pch?

test/Driver/cl-pch.cpp
368 ↗(On Diff #160365)

Can we have an integration-test type test that creates a pch and uses it, and checks that there are no warnings? (Like in https://bugs.llvm.org/show_bug.cgi?id=38508#c1) Probably fairly easy to add to one of the test/PCH/pch-through* files.

This revision is now accepted and ready to land.Aug 15 2018, 12:37 PM
mikerice updated this revision to Diff 161116.Aug 16 2018, 2:49 PM
mikerice marked an inline comment as done.

Added a -verify test to ensure no warnings on successful PCH use.

How does the gcc driver codepath handle this?

Interestingly I'd say. So the gcc PCH model uses -include pch.h to use a PCH. In the driver, -include pch.h is handled by locating a matching pch file (say pch.h.gch) and if found adds -include_pch pch.h.gch instead of -include pch.h. This occurs even when only preprocessing (-E). When preprocessing the compiler then handles the -include_pch option by opening pch.h.gch, finding the original file that created it (pch.h) and doing what -include pch.h does. Seems like it would be better handled by the driver but maybe there is a reason for it.

When we only allowed PCH with a single /FI the model was the same as gcc and it worked similarly. Since the through header/MS model allows creation of a PCH from any tokens not just a single #include, we can't handle it this way anymore.

thakis accepted this revision.Aug 16 2018, 3:19 PM

Thanks! Do you need someone to land this?

Thanks! Do you need someone to land this?

Feel free to commit it if you want. Otherwise I'll have my colleague take care of it in the morning. Thanks!

This revision was automatically updated to reflect the committed changes.