Re: [PATCH] D61914: [Support][Test] Time profiler: add regression test

[Puyan Lotfi via cfe-commits]

--target=

Sent from ProtonMail Mobile

On Fri, Jun 7, 2019 at 2:01 PM, Anton Afanasyev via Phabricator 
 wrote:

> anton-afanasyev marked an inline comment as done.
> anton-afanasyev added a comment.
>
> In D61914#1534508 , @plotfi wrote:
>
>> cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did 
>> you mean to pass the target explicitly ?
>
> This commit is reverted now. How can I pass the target explicitly?
>
> 
> Comment at: cfe/trunk/test/Driver/check-time-trace.cpp:1
> +// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped 
> to" \
> +// RUN: | awk '{print $NF}' | xargs cat \
> 
> plotfi wrote:
>> This test should probably have // REQUIRES: shell
> Thanks! I haven't found `REQUIRES: shell` here: 
> `https://llvm.org/docs/CommandGuide/FileCheck.html`. Does this directive mean 
> the target OS must have utils like `awk` and `xargs`?
>
> Repository:
> rL LLVM
>
> CHANGES SINCE LAST ACTION
> https://reviews.llvm.org/D61914/new/
>
> https://reviews.llvm.org/D61914-BEGIN PGP PUBLIC KEY BLOCK-
Version: Pmcrypto Golang 0.0.1 (ddacebe0)
Comment: https://protonmail.com

xsBNBFiqrSIBCADBs95WesbqXDDK3y+LLHh/EuIZUAFg6tSOCD3r7VPBjRTAw6+e
H8VztrEyPm6MffHJBCh9K9N+lE7U/jtDzdEMlPX7rxt+VbkNTadkitNxHpJQqifu
qI4SyOQfpedyIJZ/VokXgtqdBV0iugtlRvkC7kyk+H8rOwBDNEGL9eh+zL/k20RP
CNATpXl902yOMC9nCaB7zDZt/twPbUY7qGjV3wxSwIzUySdZPLgPcseip84O3uYX
+QShZAqgi5ajCJflqcQKNUMJDN0dX4tFmaOWQ4HKQ4tTFkX4x6iOFy/1lRb6irHp
g7/BXpNLeagvyjMc598+tYObSpqVHIVAqc/RABEBAAHNIXB1eWFuQHB1eWFuLm9y
ZyA8cHV5YW5AcHV5YW4ub3JnPsLAdQQQAQgAKQUCWKqtIgYLCQcIAwIJEHs5Mm+b
S7GXBBUIAgoDFgIBAhkBAhsDAh4BAAAN8gf7BVfluoQUyiIQx2ponrIRv6jeOTiv
0aYUyFwUzQ7aPdtHSORYoQNKtLbXV2jUMtrsR3MIW2eYVA51iO/V0OqYGAFtc45W
YnQ0XK0bEPjQHx/oMhCGnlcFVGt5dLymqIQN8X0AROYydx0QG8EMGIyrU5OkJOJG
grIfnImUJ5N9naBBolXoGeO4q5tZ9PewcbYWm+jVZa92CO2f6LXeEQUCDXN6GpsT
WF3O8G8fRNC89maOIZKpx23Pv9JnPA0fBWJb1vyJujygtCHkPwa4SwQw9p9bcds9
n22XRIOICNbgT3fJ19JIuVkt9guHXrJE7jLWCAd8f+7vLfYs+LreS1fwBs7ATQRY
qq0iAQgAoUEmT7Epc7aX+Ylh+eHdxeLbGizzm1wzwtQtm/f72DiqJueLC/I1RQKe
M6wTXyfdF2yZ0E0/Hf7YREnLEjqVxmD5ygS1Ifvv2gzNyAvDWc8bey3t3NRGsWvv
8nUeJNM3vhWPpRIEUZ+t0l4fLYFlm1o1lOh6T33LdiyZW62shVmuS82Vr4B223Yx
/oHtgIBD+PTXlETrOPKPdxwbUjaQkqlFlzTH2Wc1ygW8wM+rXuHQPJ8BlIIopIZt
I3ZFz3r3Zy3j4k6I0QyP7Z5j1jm3bIOXgIJeE2HqNWhpYNFAGqzrlGY+1sEH2T/A
6sfca5oe+6ziPpWJxqmou+9GgiYbPwARAQABwsBfBBgBCAATBQJYqq0iCRB7OTJv
m0uxlwIbDAAAO8cH/0UtYyy4pm2E9xfw3cz3/7NfPoY5KZH0DtivFFxmlzo07BJh
qmFVql7yE8yzK8pqssBqmsV012H2+/sqPbudShbW4UOPcnDiHAgfDb+UR8QEqmJf
bDo9Gpk6Bo3VHGbpCtlZY88cHJYpXu2g69jQIyseG0pQKGcRJZ+8t/rK3IX086Uy
ZehflSk3uU3hxdN96JlvFf4GEdgdzxlE6S3AnyjV05yrjkiDhFmSRt1Pr4U9uSES
swXnufUmP+QgdINI94GPhyJSkvdRoFk0nb7E3lCyzCgIEc2p//vfuhBMN2Znrwzp
MccpJXqocidMX0HqjMh9ZUlLkEaYeAGg+qwBVRE=
=HAbn
-END PGP PUBLIC KEY BLOCK-___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked an inline comment as done.
anton-afanasyev added a comment.

In D61914#1534508 , @plotfi wrote:

> cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did you 
> mean to pass the target explicitly ?


This commit is reverted now. How can I pass the target explicitly?




Comment at: cfe/trunk/test/Driver/check-time-trace.cpp:1
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \

plotfi wrote:
> This test should probably have // REQUIRES: shell
Thanks! I haven't found `REQUIRES: shell` here: 
`https://llvm.org/docs/CommandGuide/FileCheck.html`. Does this directive mean 
the target OS must have utils like `awk` and `xargs`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: cfe/trunk/test/Driver/check-time-trace.cpp:1
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \

This test should probably have // REQUIRES: shell


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did you 
mean to pass the target explicitly ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362792: [Support][Test] Time profiler: add regression test 
(authored by anton-afanasyev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61914?vs=203532&id=203544#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61914

Files:
  cfe/trunk/test/Driver/check-time-trace.cpp
  cfe/trunk/tools/driver/cc1_main.cpp


Index: cfe/trunk/test/Driver/check-time-trace.cpp
===
--- cfe/trunk/test/Driver/check-time-trace.cpp
+++ cfe/trunk/test/Driver/check-time-trace.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \
+// RUN:   | %python -c 'import json, sys; 
json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
+// RUN:   | FileCheck %s
+
+// CHECK: "traceEvents": [
+// CHECK: "args":
+// CHECK: "detail":
+// CHECK: "dur":
+// CHECK: "name": "Source"
+// CHECK-NEXT: "ph":
+// CHECK-NEXT: "pid":
+// CHECK-NEXT: "tid":
+// CHECK-NEXT: "ts":
+// CHECK: "name": "clang"
+// CHECK: "name": "process_name"
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: cfe/trunk/tools/driver/cc1_main.cpp
===
--- cfe/trunk/tools/driver/cc1_main.cpp
+++ cfe/trunk/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're


Index: cfe/trunk/test/Driver/check-time-trace.cpp
===
--- cfe/trunk/test/Driver/check-time-trace.cpp
+++ cfe/trunk/test/Driver/check-time-trace.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \
+// RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
+// RUN:   | FileCheck %s
+
+// CHECK: "traceEvents": [
+// CHECK: "args":
+// CHECK: "detail":
+// CHECK: "dur":
+// CHECK: "name": "Source"
+// CHECK-NEXT: "ph":
+// CHECK-NEXT: "pid":
+// CHECK-NEXT: "tid":
+// CHECK-NEXT: "ts":
+// CHECK: "name": "clang"
+// CHECK: "name": "process_name"
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: cfe/trunk/tools/driver/cc1_main.cpp
===
--- cfe/trunk/tools/driver/cc1_main.cpp
+++ cfe/trunk/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Alexandre Ganea via Phabricator via cfe-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

Thanks Anton!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev updated this revision to Diff 203532.
anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914

Files:
  clang/test/Driver/check-time-trace.cpp
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
Index: clang/test/Driver/check-time-trace.cpp
===
--- /dev/null
+++ clang/test/Driver/check-time-trace.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \
+// RUN:   | %python -c 'import json, sys; 
json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
+// RUN:   | FileCheck %s
+
+// CHECK: "traceEvents": [
+// CHECK: "args":
+// CHECK: "detail":
+// CHECK: "dur":
+// CHECK: "name": "Source"
+// CHECK-NEXT: "ph":
+// CHECK-NEXT: "pid":
+// CHECK-NEXT: "tid":
+// CHECK-NEXT: "ts":
+// CHECK: "name": "clang"
+// CHECK: "name": "process_name"
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
Index: clang/test/Driver/check-time-trace.cpp
===
--- /dev/null
+++ clang/test/Driver/check-time-trace.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat \
+// RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
+// RUN:   | FileCheck %s
+
+// CHECK: "traceEvents": [
+// CHECK: "args":
+// CHECK: "detail":
+// CHECK: "dur":
+// CHECK: "name": "Source"
+// CHECK-NEXT: "ph":
+// CHECK-NEXT: "pid":
+// CHECK-NEXT: "tid":
+// CHECK-NEXT: "ts":
+// CHECK: "name": "clang"
+// CHECK: "name": "process_name"
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 4 inline comments as done.
anton-afanasyev added a comment.

In D61914#1512021 , @aganea wrote:

> Could you please move the test to a more approriate location? (ie. 
> clang/trunk/test/Driver/)


Thanks, I've moved it there.




Comment at: clang/tools/driver/cc1_main.cpp:245
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()

aganea wrote:
> This seems a bit too chatty. Suround these two lines with `if 
> (Config->Verbose)` ?
I don't think it should be done this way for several reasons:
1. This info is actually needed by user when one uses `--ftime-trace` since 
json-file is usually dumped to random filename located in `/tmp/`. Therefore 
`--ftime-trace` means `--ftime-trace --verbose` itself. Without this info user 
cannot understand where dumped file is. How can he know about `--verbose`?
2. Option `--ftime-report` is much more chatty, it dumps several tables to 
terminal. This option just dumps to file, but tells where file is.
3. I have not found elegant way to check `--verbose` option here, one should 
add it to `clang::FrontendOptions` or so.



Comment at: llvm/test/Support/check-time-trace.cxx:4
+
+// CHECK: "args":{"name":"clang"}
+

aganea wrote:
> I don't see any other `-ftime-trace` tests, I would add a few more exhaustive 
> file format checks here.
Ok, I've added more checks, parsing json file by python.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Could you please move the test to a more approriate location? (ie. 
clang/trunk/test/Driver/)




Comment at: clang/tools/driver/cc1_main.cpp:245
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()

This seems a bit too chatty. Suround these two lines with `if 
(Config->Verbose)` ?



Comment at: llvm/test/Support/check-time-trace.cxx:4
+
+// CHECK: "args":{"name":"clang"}
+

I don't see any other `-ftime-trace` tests, I would add a few more exhaustive 
file format checks here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61914



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev created this revision.
anton-afanasyev added a reviewer: thakis.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Add output to `llvm::errs()` when `-ftime-trace` option is enabled,
add regression test checking this option works as expected.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61914

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/test/Support/check-time-trace.cxx


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits