[PATCH] clang-cl: implement /fallback mode

[Hans Wennborg]

================
Comment at: include/clang/Driver/CLCompatOptions.td:114
@@ -113,1 +113,3 @@
+def _SLASH_fallback : CLCompileFlag<"fallback">,
+  HelpText<"Fall back to cl.exe if clang-cl fails due to internal error">;
 def _SLASH_Fe : CLJoined<"Fe">,
----------------
Reid Kleckner wrote:
> Do we want to support multiple fallback modes like fallback for internal errors or fallback for any error?  I guess we can just use this and see what comes up in practice, and eventually get to a place where we only fallback on "could not compile 'foo' yet" diagnostics.
I'd like to start out with fallback on any error as the patch currently does, but I think allowing something like /fallback=internal would be a good thing to add in the future.

================
Comment at: lib/Driver/Tools.cpp:3566
@@ +3565,3 @@
+  if (Args.hasArg(options::OPT__SLASH_fallback)) {
+    // FIXME: This is where it gets crazy :)
+    tools::visualstudio::Compile CL(getToolChain());
----------------
Reid Kleckner wrote:
> I don't see any better way to do it, so this is probably OK.  No need for FIXME.  :)
Oops, yeah I didn't mean to include that in the patch :)

================
Comment at: lib/Driver/Tools.cpp:6687
@@ +6686,3 @@
+  // Flags for which clang-cl have an alias.
+  // FIXME: Could we avoid this manual reverse translation?
+
----------------
Reid Kleckner wrote:
> Can you iterate over Args.getArgString(I) up to getNumInputArgStrings(), or does that fail?
We could do that, but we don't want to just pass through the flags verbatim - we'd need to look at the option and decide whether to include it or not, and then we get into parsing the options and it could get a bit messy. As we expose more "core" clang flags we also need to take interactions with those into account.

I like the current approach of "canonicalizing" a new cl.exe command-line based on the options as clang-cl has understood them. My only worry is keeping this in sync with the supported options. At least it's easily testable.

================
Comment at: lib/Driver/Tools.cpp:6721
@@ +6720,3 @@
+
+  // FIXME: If we've put clang-cl as cl.exe on the path, then we might have a problem now :/
+  const char *Exec =
----------------
Reid Kleckner wrote:
> That seems problematic.  You could use sys::fs::equivalent() to skip a path if it's the current executable, but we'd have to sink that into GetProgramPath().
On the good side, we're not passing /fallback through to the second invocation, so we don't get stuck forever :)

On the bad side, I don't think checking sys::fs::equivalent() would work, because the cl.exe on the PATH could be a copy of clang-cl.exe, as in the case of the current VS integration :/

It seems that automatically looking for the "true" cl.exe could get pretty messy.

What do you think about having an option, like /fallbackcommand <foo>? Ideally, the VS integration could set this automagically, and it would also be useful for testing (we could set this to "echo" or something and test the actual execution of the fallback).

================
Comment at: test/Driver/cl-fallback.c:8
@@ +7,3 @@
+// RUN: %clang_cl /fallback /Dfoo=bar /Ubaz /Ifoo /Ox /O0 /GR /GR- /LD /LDd \
+// RUN:     /MD /MDd /MTd /MT -### -- %s 2>&1 | FileCheck %s
+// CHECK: ||
----------------
Reid Kleckner wrote:
> I guess that's the most testing we can do here.  Oh well.
I guess we could invent a "REQUIRES" that is true iff cl.exe is on the path, and then we could actually test that the invocation works. I don't know if we have any bots that run with that configuration though.

But I think the most important thing is to get good test coverage of the flag translation.


http://llvm-reviews.chandlerc.com/D1711