<div dir="ltr">All done. phab's back, but since we started with a patch attachment, let's keep it that way.<div><div><br></div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 12, 2016 at 12:12 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Apr 11, 2016 at 7:16 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
> r260990 exposed -isystem in clang-cl. -isystem adds a directory to the front<br>
> of the system include search path. The idea was to use this to point to a<br>
> hermetic msvc install, but as it turns out this doesn't work: -isystem then<br>
> adds the hermetic headers in front of clang's builtin headers, and clang's<br>
> headers that are supposed to wrap msvc headers (say, stdarg.h) aren't picked<br>
> up at all anymore.<br>
><br>
> So revert that, and instead expose -imsvc which works as if the passed<br>
> directory was part of %INCLUDE%: The header is treated as a system header,<br>
> but it is searched after clang's lib/Header headers.<br>
<br>
</span>Sounds great.<br>
<br>
I wonder if this is useful beyond clang-cl and should have a more<br>
general name, but I can't think of anything, so let's go with this.<br>
<span class=""><br>
> Also expose -nostdlibinc so that clang-cl can be told to not look for system<br>
> headers in the usual places.<br>
<br>
</span>Can you land this in a separate commit?<br>
<span class=""><br>
<br>
> With this change, it's possible to use -imsvc to point clang-cl at a<br>
> hermetic MSVC installation and not make it look at the headers of a possibly<br>
> installed msvc. Fixes PR26751.<br>
><br>
> I'd upload this to phab, but it's currently down.<br>
<br>
</span>Old-school review below:<br>
<br>
> Index: include/clang/Driver/CLCompatOptions.td<br>
> ===================================================================<br>
> --- include/clang/Driver/CLCompatOptions.td (revision 265325)<br>
> +++ include/clang/Driver/CLCompatOptions.td (working copy)<br>
> @@ -210,6 +210,8 @@<br>
>    HelpText<"Enable exception handling">;<br>
>  def _SLASH_GX_ : CLFlag<"GX-">,<br>
>    HelpText<"Enable exception handling">;<br>
> +def _SLASH_imsvc : CLJoinedOrSeparate<"imsvc">,<br>
> +  HelpText<"Add directory to system include search path, as if part of %INCLUDE%">, MetaVarName<"<dir>">;<br>
<br>
Nit: I'd have gone with a line break before MetaVarName to pretend<br>
we're at least trying to keep the lines short.<br>
<br>
> Index: include/clang/Driver/Options.td<br>
> ===================================================================<br>
> --- include/clang/Driver/Options.td (revision 265325)<br>
> +++ include/clang/Driver/Options.td (working copy)<br>
> @@ -1246,7 +1246,7 @@<br>
>  def isysroot : JoinedOrSeparate<["-"], "isysroot">, Group<clang_i_Group>, Flags<[CC1Option]>,<br>
>    HelpText<"Set the system root directory (usually /)">, MetaVarName<"<dir>">;<br>
>  def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,<br>
> -  Flags<[CC1Option, CoreOption]>,<br>
> +  Flags<[CC1Option]>,<br>
>    HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;<br>
>  def iwithprefixbefore : JoinedOrSeparate<["-"], "iwithprefixbefore">, Group<clang_i_Group>,<br>
>    HelpText<"Set directory to include search path with prefix">, MetaVarName<"<dir>">,<br>
> @@ -1673,7 +1673,7 @@<br>
>  def noseglinkedit : Flag<["-"], "noseglinkedit">;<br>
>  def nostartfiles : Flag<["-"], "nostartfiles">;<br>
>  def nostdinc : Flag<["-"], "nostdinc">;<br>
> -def nostdlibinc : Flag<["-"], "nostdlibinc">;<br>
> +def nostdlibinc : Flag<["-"], "nostdlibinc">, Flags<[CoreOption]>;<br>
<br>
Commit this one separately.<br>
<br>
> Index: lib/Driver/MSVCToolChain.cpp<br>
> ===================================================================<br>
> --- lib/Driver/MSVCToolChain.cpp (revision 265325)<br>
> +++ lib/Driver/MSVCToolChain.cpp (working copy)<br>
> @@ -527,6 +527,10 @@<br>
>                                    "include");<br>
>    }<br>
><br>
> +  // Add %INCLUDE%-like directories from the -systemI flag.<br>
> +  for (const auto &path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))<br>
> +    addSystemInclude(DriverArgs, CC1Args, path);<br>
<br>
Update flag name in the comment. Spelling the loop variable Path would<br>
be more LLVM-style.<br>
<br>
> Index: test/Driver/cl-options.c<br>
> ===================================================================<br>
> --- test/Driver/cl-options.c (revision 265325)<br>
> +++ test/Driver/cl-options.c (working copy)<br>
> @@ -82,6 +82,10 @@<br>
>  // RUN: %clang_cl /I myincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_I %s<br>
>  // SLASH_I: "-I" "myincludedir"<br>
><br>
> +// RUN: %clang_cl /imsvcmyincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_imsvc %s<br>
> +// RUN: %clang_cl /imsvc myincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_imsvc %s<br>
> +// SLASH_imsvc: "-internal-isystem" "myincludedir"<br>
<br>
Would it be possible to check that this flag comes after the<br>
-internal-isystem for the resource headers?<br>
</blockquote></div><br></div>