[clang-cl] Ignore the /o option when /P is specified.

[gbedwell]

Summary

[clang-cl] Ignore the /o option when /P is specified.

This matches the cl.exe behavior (tested with 18.00.31101).  In order to
specify an output file for /P, use the /Fi option instead.

I ran into this when my change to add Windows version information to LLVM executables caused a failure on the clang selfhost builds. This is because the CMake tool cmcldeps.exe which it's using to figure out dependencies for the resource script file is invoking the host C++ compiler with '/P /out:<location>' but expecting <location> to be ignored. With MSVC's cl.exe (I tested VS2012 and VS2013) this is the case, but with clang-cl.exe we're trying to output the file to <location>. This patch changes clang-cl to match the cl behaviour in ignoring /o which allows cmcldeps.exe tool to function correctly with clang-cl. The test ensures that the '/Fi' option still works as expected to allow the location of the /P output to be specified if required.

Note that I mentioned that cmcldeps.exe is specifying the location with '/out:<location>', rather than '/o' '<location>'. This means that in this case it's actually being parsed as '/o' 'ut:<location>' but as it's being ignored anyway it doesn't cause us any problems. I'm not sure whether we'd also want to add '/out:' as an alias for '/o' too. As both '/o' and '/out:' now give a warning from cl.exe:

"Command line warning D9035 : option 'o' has been deprecated and will be removed in a future release"

I'm inclined to think that we shouldn't add '/out:' support as I'm not aware of any particular use-cases where it's required but I can do that too in a separate patch if the consensus is that we should.

Thanks,
-Greg

[gbedwell]

Actually, looking at it more I'm not even sure that MSVC supports the '/out:' form except for in the linker. I think cl.exe may also be interpreting the command line from CMake's cmcldeps as '/o' 'ut:<location>'. Either way, it is ignored so the behaviour after this patch is correct.

[• ehsan]

http://llvm.org/PR20894 is a good read here.

I'm fine with this change myself, but I would be interested to know what
Hans thinks.

[hans]

LGTM with comment.

test/Driver/cl-outputs.c

266

We probably don't need all four test cases for /P with /o anymore.

I think we should also have a note explaining the expected behaviour: that /o doesn't affect /P.

lib/Driver/Driver.cpp

if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
       return C.addResultFile(FinalOutput->getValue(), &JA);
   }
 
   // For /P, preprocess to file named after BaseInput.
   if (C.getArgs().hasArg(options::OPT__SLASH_P)) {
     assert(AtTopLevel && isa<PreprocessJobAction>(JA));
     StringRef BaseName = llvm::sys::path::filename(BaseInput);
     StringRef NameArg;
-    if (Arg *A = C.getArgs().getLastArg(options::OPT__SLASH_Fi,
-                                        options::OPT__SLASH_o))
+    if (Arg *A = C.getArgs().getLastArg(options::OPT__SLASH_Fi))
       NameArg = A->getValue();
     return C.addResultFile(MakeCLOutputFilename(C.getArgs(), NameArg, BaseName,
                                                 types::TY_PP_C), &JA);
   }
 
   // Default to writing to stdout?
   if (AtTopLevel && !CCGenDiagnostics &&
       (isa<PreprocessJobAction>(JA) || JA.getType() == types::TY_ModuleFile))

test/Driver/cl-outputs.c

 // Fi1: "-o" "foo.i"
 
 // RUN: %clang_cl /P /Fifoo.x -### -- %s 2>&1 | FileCheck -check-prefix=Fi2 %s
 // Fi2: "-E"
 // Fi2: "-o" "foo.x"
 
 // RUN: %clang_cl /P /ofoo -### -- %s 2>&1 | FileCheck -check-prefix=Fio1 %s
 // Fio1: "-E"
-// Fio1: "-o" "foo.i"
+// Fio1: "-o" "cl-outputs.i"
 
 // RUN: %clang_cl /P /o foo -### -- %s 2>&1 | FileCheck -check-prefix=Fio2 %s
 // Fio2: "-E"
-// Fio2: "-o" "foo.i"
+// Fio2: "-o" "cl-outputs.i"
 
 // RUN: %clang_cl /P /ofoo.x -### -- %s 2>&1 | FileCheck -check-prefix=Fio3 %s
 // Fio3: "-E"
-// Fio3: "-o" "foo.x"
+// Fio3: "-o" "cl-outputs.i"
 
 // RUN: %clang_cl /P /o foo.x -### -- %s 2>&1 | FileCheck -check-prefix=Fio4 %s
 // Fio4: "-E"
-// Fio4: "-o" "foo.x"
+// Fio4: "-o" "cl-outputs.i"
 
 
 // RUN: %clang_cl /P /obar.x /Fifoo.x -### -- %s 2>&1 | FileCheck -check-prefix=FioRACE1 %s
 // FioRACE1: "-E"
 // FioRACE1: "-o" "foo.x"
 
 // RUN: %clang_cl /P /Fifoo.x /obar.x -### -- %s 2>&1 | FileCheck -check-prefix=FioRACE2 %s
 // FioRACE2: "-E"
-// FioRACE2: "-o" "bar.x"
+// FioRACE2: "-o" "foo.x"
 
 // RUN: %clang_cl /c /GL -### -- %s 2>&1 | FileCheck -check-prefix=LTO-DEFAULT %s
 // LTO-DEFAULT: "-emit-llvm-bc"{{.*}}"-o" "cl-outputs.obj"
 
 // RUN: %clang_cl /c /GL /Fofoo -### -- %s 2>&1 | FileCheck -check-prefix=LTO-FO %s
 // LTO-FO: "-emit-llvm-bc"{{.*}}"-o" "foo.obj"