Page MenuHomePhabricator

Replace CLANG_SPAWN_CC1 env var with a driver mode flag
ClosedPublic

Authored by thakis on Jan 15 2020, 7:51 AM.

Details

Summary

Flags are clang's default UI.

We can have an env var in addition to that, but in D69825 nobody has yet mentioned why this needs an env var, so omit it for now.
If someone needs to set the flag via env var, the existing CCC_OVERRIDE_OPTIONS mechanism works
for it (set CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 for example).

Also mention the cc1-in-process change in the release notes.

Also spruce up the test a bit so it actually tests something :)

Diff Detail

Event Timeline

thakis created this revision.Jan 15 2020, 7:51 AM
thakis edited the summary of this revision. (Show Details)

I guess CLANG_SPAWN_CC1 is/was mostly for debugging purposes. We have otherwise no use for that env var in production, other than for occasional testing.

You could run your build, and toggle that env var without changing the build options.
In the same way, I was also using it when building LLVM with ninja check-all to ensure things work in the same way (when CLANG_SPAWN_CC1=0 or =1). You would otherwise need to fully rebuild LLVM if there's no env var.

The other issue I was seeing with a flag is that, it could be enabled on some libraries, but not others, depending on how the build system is setup. For example, if you're using an external package which is generated from an external script, but compiled within your project, with your clang-cl, you could easily miss that flag (especially when you have tens of external packages to update).

thakis edited the summary of this revision. (Show Details)Jan 15 2020, 8:19 AM

I guess CLANG_SPAWN_CC1 is/was mostly for debugging purposes. We have otherwise no use for that env var in production, other than for occasional testing.

It's a pretty major change in how clang runs compilations, and I think we need some way to turn it off in case it causes issues somewhere (distcc or similar). Every build system can easily change flags. Some can't easily add env vars. We definitely need a flag for this.

You could run your build, and toggle that env var without changing the build options.
In the same way, I was also using it when building LLVM with ninja check-all to ensure things work in the same way (when CLANG_SPAWN_CC1=0 or =1). You would otherwise need to fully rebuild LLVM if there's no env var.

That's a good point. Clang has an existing mechanism for this (set CCC_OVERRIDE_OPTIONS=^-fno-integrated-cc1 like you would for any other flag), but it didn't work for this CL. It now does.

The other issue I was seeing with a flag is that, it could be enabled on some libraries, but not others, depending on how the build system is setup. For example, if you're using an external package which is generated from an external script, but compiled within your project, with your clang-cl, you could easily miss that flag (especially when you have tens of external packages to update).

That's true for every flag under the sun though :)

make flag work with CCC_OVERRIDE_OPTIONS

add CCC_OVERRIDE_OPTIONS=^-fno-integrated-cc1 test

thakis edited the summary of this revision. (Show Details)Jan 15 2020, 8:41 AM
hans accepted this revision.Jan 15 2020, 8:56 AM

lgtm (with comment)

Having a flag makes sense, and those wanting an env var can use CCC_OVERRIDE_OPTIONS, so everyone is happy :-)

clang/tools/driver/driver.cpp
288

This has the same arg passed twice?

This revision is now accepted and ready to land.Jan 15 2020, 8:56 AM
thakis marked an inline comment as done.Jan 15 2020, 9:22 AM
thakis added inline comments.
clang/tools/driver/driver.cpp
288

Errrrrr. Fixed, and added a test for this too. Thanks!

cc cfe-commits for completeness

Thanks for fixing this.

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

(Happy to file a bug, just want to check first)

hans added a comment.Jan 16 2020, 1:07 AM

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

Yes, this is on my list. Thanks for keeping the release branch in mind :-)

hans added a comment.Jan 16 2020, 4:23 AM

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

Yes, this is on my list. Thanks for keeping the release branch in mind :-)

Cherry-picked to 10.x in c4a134a

hans added a comment.Jan 16 2020, 4:25 AM

Oh, and removed the release note from trunk in 00c74d0b644