[patch] clang-cl: Remove -isystem, add -imsvc, expose -nostdlibinc

[Hans Wennborg via cfe-commits]

On Mon, Apr 11, 2016 at 7:16 PM, Nico Weber <thakis at chromium.org> wrote:
> r260990 exposed -isystem in clang-cl. -isystem adds a directory to the front
> of the system include search path. The idea was to use this to point to a
> hermetic msvc install, but as it turns out this doesn't work: -isystem then
> adds the hermetic headers in front of clang's builtin headers, and clang's
> headers that are supposed to wrap msvc headers (say, stdarg.h) aren't picked
> up at all anymore.
>
> So revert that, and instead expose -imsvc which works as if the passed
> directory was part of %INCLUDE%: The header is treated as a system header,
> but it is searched after clang's lib/Header headers.

Sounds great.

I wonder if this is useful beyond clang-cl and should have a more
general name, but I can't think of anything, so let's go with this.

> Also expose -nostdlibinc so that clang-cl can be told to not look for system
> headers in the usual places.

Can you land this in a separate commit?


> With this change, it's possible to use -imsvc to point clang-cl at a
> hermetic MSVC installation and not make it look at the headers of a possibly
> installed msvc. Fixes PR26751.
>
> I'd upload this to phab, but it's currently down.

Old-school review below:

> Index: include/clang/Driver/CLCompatOptions.td
> ===================================================================
> --- include/clang/Driver/CLCompatOptions.td (revision 265325)
> +++ include/clang/Driver/CLCompatOptions.td (working copy)
> @@ -210,6 +210,8 @@
>    HelpText<"Enable exception handling">;
>  def _SLASH_GX_ : CLFlag<"GX-">,
>    HelpText<"Enable exception handling">;
> +def _SLASH_imsvc : CLJoinedOrSeparate<"imsvc">,
> +  HelpText<"Add directory to system include search path, as if part of %INCLUDE%">, MetaVarName<"<dir>">;

Nit: I'd have gone with a line break before MetaVarName to pretend
we're at least trying to keep the lines short.

> Index: include/clang/Driver/Options.td
> ===================================================================
> --- include/clang/Driver/Options.td (revision 265325)
> +++ include/clang/Driver/Options.td (working copy)
> @@ -1246,7 +1246,7 @@
>  def isysroot : JoinedOrSeparate<["-"], "isysroot">, Group<clang_i_Group>, Flags<[CC1Option]>,
>    HelpText<"Set the system root directory (usually /)">, MetaVarName<"<dir>">;
>  def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,
> -  Flags<[CC1Option, CoreOption]>,
> +  Flags<[CC1Option]>,
>    HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;
>  def iwithprefixbefore : JoinedOrSeparate<["-"], "iwithprefixbefore">, Group<clang_i_Group>,
>    HelpText<"Set directory to include search path with prefix">, MetaVarName<"<dir>">,
> @@ -1673,7 +1673,7 @@
>  def noseglinkedit : Flag<["-"], "noseglinkedit">;
>  def nostartfiles : Flag<["-"], "nostartfiles">;
>  def nostdinc : Flag<["-"], "nostdinc">;
> -def nostdlibinc : Flag<["-"], "nostdlibinc">;
> +def nostdlibinc : Flag<["-"], "nostdlibinc">, Flags<[CoreOption]>;

Commit this one separately.

> Index: lib/Driver/MSVCToolChain.cpp
> ===================================================================
> --- lib/Driver/MSVCToolChain.cpp (revision 265325)
> +++ lib/Driver/MSVCToolChain.cpp (working copy)
> @@ -527,6 +527,10 @@
>                                    "include");
>    }
>
> +  // Add %INCLUDE%-like directories from the -systemI flag.
> +  for (const auto &path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
> +    addSystemInclude(DriverArgs, CC1Args, path);

Update flag name in the comment. Spelling the loop variable Path would
be more LLVM-style.

> Index: test/Driver/cl-options.c
> ===================================================================
> --- test/Driver/cl-options.c (revision 265325)
> +++ test/Driver/cl-options.c (working copy)
> @@ -82,6 +82,10 @@
>  // RUN: %clang_cl /I myincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_I %s
>  // SLASH_I: "-I" "myincludedir"
>
> +// RUN: %clang_cl /imsvcmyincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_imsvc %s
> +// RUN: %clang_cl /imsvc myincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_imsvc %s
> +// SLASH_imsvc: "-internal-isystem" "myincludedir"

Would it be possible to check that this flag comes after the
-internal-isystem for the resource headers?