[Driver] Escape the program path for -frecord-command-line

[ravi-ramaseshan]

Summary

Similar to the rest of the command line that is recorded, the program
path must also have spaces and backslashes escaped. Without this
parsing the recorded command line becomes hard on platforms like
Windows where spaces and backslashes are common.

[scott.linder]

This LGTM, but could you add a simple test with a command-line that needs the escaping?

[ravi-ramaseshan]

Add a simple test with a command-line that needs the escaping.

[ravi-ramaseshan]

In D74811#1882783, @scott.linder wrote:

This LGTM, but could you add a simple test with a command-line that needs the escaping?

Done.

[scott.linder]

LGTM, thanks!

[ravi-ramaseshan]

Can you please land this change for me since I do not have commit rights. Thanks!

[scott.linder]

Multiple build bots were failing with the patch applied, see e.g. http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4890/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Aclang_f_opts.c

Seems like on platforms with a backslash for a directory separator you get "\\\\with\\ spaces\\\\clang" rather than "/with\\ spaces/clang", although I'm not completely sure that is what we want? I imagine platforms with \ as a separator have different escaping rules?

I reverted the commit for now; @ravi-ramaseshan is it OK if I just amend the test to look for "-record-command-line" "{{.+}}with\\ spaces{{.+}}" and we can worry about whether the other escaping is correct separately?

[ravi-ramaseshan]

In D74811#1885514, @scott.linder wrote:

Multiple build bots were failing with the patch applied, see e.g. http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4890/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Aclang_f_opts.c

Seems like on platforms with a backslash for a directory separator you get "\\\\with\\ spaces\\\\clang" rather than "/with\\ spaces/clang", although I'm not completely sure that is what we want? I imagine platforms with \ as a separator have different escaping rules?

I reverted the commit for now; @ravi-ramaseshan is it OK if I just amend the test to look for "-record-command-line" "{{.+}}with\\ spaces{{.+}}" and we can worry about whether the other escaping is correct separately?

Hi Scott, thanks for checking. Sorry about not testing it on those platforms. I think "\\\\with\\ spaces\\\\clang" looks correct. That's what we would expect on those platforms. We could use "{{.+}}{{/|\\\\\\\\}}with\\ spaces{{/|\\\\\\\\}}clang{{.+}}" to be more deliberate that this is what we want.

I think amending the commit as you did looks good to me. Will you be able to amend the commit for this or should I post another review with the fix? I verified that on Windows your suggestion works.

[scott.linder]

Sorry for the delay; I can just commit this, no need to update the review. I will hopefully be back to a terminal to push it a bit later tonight.

[scott.linder]

Recommitted as 340feac6721e840f88c1e77dd79931eea5eaccf3, hopefully this version sticks, cheers!

clang/lib/Driver/ToolChains/Clang.cpp

D.Diag(diag::err_drv_unsupported_opt_for_target)
         << Args.getLastArg(options::OPT_frecord_command_line)->getAsString(Args)
         << TripleStr;
   if (TC.UseDwarfDebugFlags() || GRecordSwitches || FRecordSwitches) {
     ArgStringList OriginalArgs;
     for (const auto &Arg : Args)
       Arg->render(Args, OriginalArgs);
 
     SmallString<256> Flags;
-    Flags += Exec;
+    EscapeSpacesAndBackslashes(Exec, Flags);
     for (const char *OriginalArg : OriginalArgs) {
       SmallString<128> EscapedArg;
       EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
       Flags += " ";
       Flags += EscapedArg;
     }
     auto FlagsArgString = Args.MakeArgString(Flags);
     if (TC.UseDwarfDebugFlags() || GRecordSwitches) {
void ClangAs::ConstructJob(Compilation &C, const JobAction &JA,
   // analysis.
   if (getToolChain().UseDwarfDebugFlags()) {
     ArgStringList OriginalArgs;
     for (const auto &Arg : Args)
       Arg->render(Args, OriginalArgs);
 
     SmallString<256> Flags;
     const char *Exec = getToolChain().getDriver().getClangProgramPath();
-    Flags += Exec;
+    EscapeSpacesAndBackslashes(Exec, Flags);
     for (const char *OriginalArg : OriginalArgs) {
       SmallString<128> EscapedArg;
       EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
       Flags += " ";
       Flags += EscapedArg;
     }
     CmdArgs.push_back("-dwarf-debug-flags");
     CmdArgs.push_back(Args.MakeArgString(Flags));

clang/test/Driver/clang_f_opts.c

 // RUN: %clang -### -S -target x86_64-unknown-linux -fno-record-command-line -frecord-command-line %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES %s
 // RUN: %clang -### -S -target x86_64-unknown-linux -frecord-command-line -fno-record-command-line %s 2>&1 | FileCheck -check-prefix=CHECK-NO-RECORD-GCC-SWITCHES %s
 // Test with a couple examples of non-ELF object file formats
 // RUN: %clang -### -S -target x86_64-unknown-macosx -frecord-command-line %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES-ERROR %s
 // RUN: %clang -### -S -target x86_64-unknown-windows -frecord-command-line %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES-ERROR %s
 // CHECK-RECORD-GCC-SWITCHES: "-record-command-line"
 // CHECK-NO-RECORD-GCC-SWITCHES-NOT: "-record-command-line"
 // CHECK-RECORD-GCC-SWITCHES-ERROR: error: unsupported option '-frecord-command-line' for target
+// Test when clang is in a path containing a space.
+// RUN: mkdir -p "%t.r/with spaces"
+// RUN: cp %clang "%t.r/with spaces/clang"
+// RUN: "%t.r/with spaces/clang" -### -S -target x86_64-unknown-linux -frecord-gcc-switches %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES-ESCAPED %s
+// CHECK-RECORD-GCC-SWITCHES-ESCAPED: "-record-command-line" "{{.+}}/with\\ spaces/clang {{.+}}"
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=uninitialized %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-UNINIT %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=pattern %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-BAD %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
 
 // RUN: %clang -### -S -fno-temp-file %s 2>&1 | FileCheck -check-prefix=CHECK-NO-TEMP-FILE %s
 // CHECK-NO-TEMP-FILE: "-fno-temp-file"