[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan requested changes to this revision.
ravi-ramaseshan added inline comments.



Comment at: clang/tools/clang-stat-cache/clang-stat-cache.cpp:28
+#include 
+#include 
+

Some of these includes may not be available on a platform like Windows. Can you 
please guard them appropriately so that the build does not break?



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:3019
+
+  __attribute__((unused)) bool Consumed = Path.consume_front(BaseDir);
+  assert(Consumed && "Path does not start with expected prefix.");

Please use `LLVM_ATTRIBUTE_UNUSED`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136651/new/

https://reviews.llvm.org/D136651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan created this revision.
ravi-ramaseshan added reviewers: scott.linder, rjmccall.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74811

Files:
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5880,7 +5880,7 @@
   Arg->render(Args, OriginalArgs);
 
 SmallString<256> Flags;
-Flags += Exec;
+EscapeSpacesAndBackslashes(Exec, Flags);
 for (const char *OriginalArg : OriginalArgs) {
   SmallString<128> EscapedArg;
   EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
@@ -6788,7 +6788,7 @@
 
 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);


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5880,7 +5880,7 @@
   Arg->render(Args, OriginalArgs);
 
 SmallString<256> Flags;
-Flags += Exec;
+EscapeSpacesAndBackslashes(Exec, Flags);
 for (const char *OriginalArg : OriginalArgs) {
   SmallString<128> EscapedArg;
   EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
@@ -6788,7 +6788,7 @@
 
 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan updated this revision to Diff 245456.
ravi-ramaseshan added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74811/new/

https://reviews.llvm.org/D74811

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/clang_f_opts.c


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -581,6 +581,11 @@
 // 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5880,7 +5880,7 @@
   Arg->render(Args, OriginalArgs);
 
 SmallString<256> Flags;
-Flags += Exec;
+EscapeSpacesAndBackslashes(Exec, Flags);
 for (const char *OriginalArg : OriginalArgs) {
   SmallString<128> EscapedArg;
   EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
@@ -6788,7 +6788,7 @@
 
 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);


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -581,6 +581,11 @@
 // 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5880,7 +5880,7 @@
   Arg->render(Args, OriginalArgs);
 
 SmallString<256> Flags;
-Flags += Exec;
+EscapeSpacesAndBackslashes(Exec, Flags);
 for (const char *OriginalArg : OriginalArgs) {
   SmallString<128> EscapedArg;
   EscapeSpacesAndBackslashes(OriginalArg, EscapedArg);
@@ -6788,7 +6788,7 @@
 
 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan added a comment.

In D74811#1882783 , @scott.linder 
wrote:

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


Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74811/new/

https://reviews.llvm.org/D74811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74811/new/

https://reviews.llvm.org/D74811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ravi Ramaseshan via Phabricator via cfe-commits]

ravi-ramaseshan added a comment.

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\\ spacesclang"` 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\\ spacesclang" looks correct.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74811/new/

https://reviews.llvm.org/D74811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits