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

[Hans Wennborg via cfe-commits]

LGTM

On Tue, Apr 12, 2016 at 11:40 AM, Nico Weber <thakis at chromium.org> wrote:
> All done. phab's back, but since we started with a patch attachment, let's
> keep it that way.
>
>
>
> On Tue, Apr 12, 2016 at 12:12 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> 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?
>
>