Page MenuHomePhabricator

[clang-cl] Implement /external:I, /external:env, and EXTERNAL_INCLUDE support (PR36003)
ClosedPublic

Authored by hans on Jun 16 2021, 8:02 AM.

Details

Summary

This patch does three things:

  • Map the /external:I flag to -isystem
  • Add support for the /external:env:<var> flag which reads system include paths for the <var> environment variable
  • Pick up system include dirs EXTERNAL_INCLUDE in addition to the old INCLUDE environment variable.

Diff Detail

Event Timeline

hans created this revision.Jun 16 2021, 8:02 AM
hans requested review of this revision.Jun 16 2021, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 8:02 AM

Nice, that's much less convoluted than I had feared :)

clang/lib/Driver/ToolChains/MSVC.cpp
1255

/external:env should happen after winsysroot I think. sysroots try to make builds hermetic and env vars defeat that.

I.e. this flag is more like the env var reads below than imsvc above – it reads the env mostly, instead of being a flag mostly.

1272

Maybe this should grow a FIXME to make regular INCLUDE not a system include at some point? Kind of sounds like this is the direction msvc is moving in with the existence of EXTERNAL_INCLUDE at least.

clang/test/Driver/cl-include.c
10

Should there be tests for the interaction with /X, /winsysroot:?

hans updated this revision to Diff 352695.Jun 17 2021, 6:18 AM
hans marked 2 inline comments as done.

More tests, add help text for command-line options.

hans added inline comments.Jun 17 2021, 6:18 AM
clang/lib/Driver/ToolChains/MSVC.cpp
1255

Hmm. I guess it depends on how people will use this flag, which isn't entirely clear.

The way I think about it, users might use this to point to third-party libraries besides the system headers. For that reason it's different from the env var reads below, which are about finding the system headers.

In particular, I don't think /nostdlibinc (/X) should disable these flags, which is why I put it before that check.

I don't think people would combine this with /winsysroot, but if they want to, I'm not sure we should prevent it.

clang/test/Driver/cl-include.c
10

Yes, adding that.

thakis accepted this revision.Jun 17 2021, 5:13 PM

That's a good argument. lgtm.

clang/lib/Driver/ToolChains/MSVC.cpp
1255

I don't know what people in general want, but _I_ definitely want a mode that makes the compiler not read any env vars :) But fair enough, this flag explicitly opts in to reading env vars, so if I don't want that I can just not pass this flag.

1279

Why not keep the definition of include_var in the if condition like it was on the rhs? (And do it for ext_include_var too)? They're only used in the respective if body, right? i.e. like so:

if (llvm::Optional<std::string> include_var =
        llvm::sys::Process::GetEnv("INCLUDE")) {
  StringRef(*include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}
if (llvm::Optional<std::string> ext_include_var =
        llvm::sys::Process::GetEnv("EXTERNAL_INCLUDE")) {
  StringRef(*ext_include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}

(Might even do for (auto s : {"INCLUDE", "EXTERNAL_INCLUDE"}) ... but that makes the FIXME harder to do later, so maybe don't do that part :) )

This revision is now accepted and ready to land.Jun 17 2021, 5:13 PM
hans added inline comments.Jun 21 2021, 6:34 AM
clang/lib/Driver/ToolChains/MSVC.cpp
1279

It's because split() creates StringRefs into include_var and ext_include_var, so they need to stay in scope until we're done with the Dirs object.

But that's pretty subtle, and the code doesn't look very nice. We're doing the same "get env var and split" dance three times now, so I'll move it to a helper lambda.