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

[Nico Weber via cfe-commits]

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?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160412/8f003bbf/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-cl-imsvc.patch
Type: application/octet-stream
Size: 3738 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160412/8f003bbf/attachment-0001.obj>