Re: [PATCH] D24820: Add -stats-file option

2016-09-23 Thread Matthias Braun via cfe-commits 
MatzeB updated this revision to Diff 72376.
MatzeB marked an inline comment as done.
MatzeB added a comment.

Thanks for the reviews.

Addressed comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D24820

Files:
  docs/CommandGuide/clang.rst
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/FrontendOptions.h
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Driver/save-stats.c
  test/Frontend/stats-file.c
  test/Misc/warning-flags.c

Index: test/Misc/warning-flags.c
===
--- test/Misc/warning-flags.c
+++ test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (84):
+CHECK: Warnings without flags (85):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -66,6 +66,7 @@
 CHECK-NEXT:   warn_fe_cc_log_diagnostics_failure
 CHECK-NEXT:   warn_fe_cc_print_header_failure
 CHECK-NEXT:   warn_fe_macro_contains_embedded_newline
+CHECK-NEXT:   warn_fe_unable_to_open_stats_file
 CHECK-NEXT:   warn_file_asm_volatile
 CHECK-NEXT:   warn_ignoring_ftabstop_value
 CHECK-NEXT:   warn_implements_nscopying
Index: test/Frontend/stats-file.c
===
--- /dev/null
+++ test/Frontend/stats-file.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -emit-llvm -o /dev/null -stats-file=%t %s
+// RUN: FileCheck -input-file=%t %s
+// CHECK: {
+//  ... here come some json values ...
+// CHECK: }
+
+// RUN: %clang_cc1 -emit-llvm -o %t -stats-file=%S/doesnotexist/bla %s 2>&1 | FileCheck -check-prefix=OUTPUTFAIL %s
+// OUTPUTFAIL: warning: unable to open statistic output file '{{.*}}{{[/\\]}}doesnotexist{{[/\\]}}bla': '{{[Nn]}}o such file or directory'
Index: test/Driver/save-stats.c
===
--- /dev/null
+++ test/Driver/save-stats.c
@@ -0,0 +1,20 @@
+// RUN: %clang -target x86_64-apple-darwin -save-stats %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin -save-stats=cwd %s -### 2>&1 | FileCheck %s
+// CHECK: "-stats-file=save-stats.stats"
+// CHECK: "{{.*}}save-stats.c"
+
+// RUN: %clang -target x86_64-apple-darwin -S %s -### 2>&1 | FileCheck %s -check-prefix=NO-STATS
+// NO-STATS-NO: -stats-file
+// NO-STATS: "{{.*}}save-stats.c"
+// NO-STATS-NO: -stats-file
+
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c -o obj/dir/save-stats.o %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ
+// CHECK-OBJ: "-stats-file=obj/dir{{.}}save-stats.stats"
+// CHECK-OBJ: "-o" "obj/dir{{.}}save-stats.o"
+
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ-NOO
+// CHECK-OBJ-NOO: "-stats-file=save-stats.stats"
+// CHECK-OBJ-NOO: "-o" "save-stats.o"
+
+// RUN: %clang -target x86_64-apple-darwin -save-stats=bla -c %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID
+// CHECK-INVALID: invalid value 'bla' in '-save-stats=bla'
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -194,8 +194,10 @@
   }
 
   ~AnalysisConsumer() override {
-if (Opts->PrintStats)
+if (Opts->PrintStats) {
   delete TUTotalTimer;
+  llvm::PrintStatistics();
+}
   }
 
   void DigestAnalyzerOptions() {
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1252,6 +1252,7 @@
   Opts.AuxTriple =
   llvm::Triple::normalize(Args.getLastArgValue(OPT_aux_triple));
   Opts.FindPchSource = Args.getLastArgValue(OPT_find_pch_source_EQ);
+  Opts.StatsFile = Args.getLastArgValue(OPT_stats_file);
 
   if (const Arg *A = Args.getLastArg(OPT_arcmt_check,
  OPT_arcmt_modify,
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -858,7 +858,7 @@
   if (getFrontendOpts().ShowTimers)
 createFrontendTimer();
 
-  if (getFrontendOpts().ShowStats)
+  if (getFrontendOpts().ShowStats || !getFrontendOpts().StatsFile.empty())
 llvm::EnableStatistics();
 
   for (const FrontendInputFile &FIF : getFrontendOpts().Inputs) {
@@ -892,9 +892,24 @@
   OS << " generated.\n";
   }
 
-  if (getFrontendOpts().ShowStats && hasFileManager()) {
-getFileManager().PrintStats();
-OS << "\n";
+  if (getFrontendOpts().ShowStats) {
+if (has

Re: [PATCH] D24820: Add -stats-file option

2016-09-23 Thread Bruno Cardoso Lopes via cfe-commits 
bruno added a comment.

Maybe add some docs to explain the new flags?



Comment at: lib/Driver/Tools.cpp:6102
@@ +6101,3 @@
+StatsFile.assign(Output.getFilename());
+llvm::sys::path::remove_filename(StatsFile);
+  }

MatzeB wrote:
> bruno wrote:
> > Why removing StatsFile here? IIUC, at this point StatsFile is still the 
> > same as the output (if it's a file).
> a) It behaves like -save-temps=obj (`clang -save-temps=obj foo.c -o bar` will 
> give you foo.i, foo.o, ...;
> b) This makes sense when you have multiple (`clang -save-stats=obj foo.c 
> bar.c -o baz`) inputs for which you need multiple .stats filenames but you 
> only have 1 output name 
> c) It magically works for `-o -` as well
> 
Ok, I see now, I misread `remove_filename` as `remove`


Comment at: test/Driver/save-stats.c:1
@@ +1,2 @@
+// RUN: %clang -target x86_64-apple-darwin -save-stats %s -### 2>&1 | 
FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin -save-stats=cwd %s -### 2>&1 | 
FileCheck %s

Is -save-stats == -save-stats=cwd? It doesn't seem so by looking at 
lib/Driver/Tools.cpp. Need test for the diag::err_drv_invalid_value as well.


Comment at: test/Driver/save-stats.c:12
@@ +11,3 @@
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c -o 
obj/dir/save-stats.o %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ
+// CHECK-OBJ: "-stats-file=obj/dir{{/|}}save-stats.stats"
+// CHECK-OBJ: "-o" "obj/dir{{/|}}save-stats.o"

aprantl wrote:
> This is up to taste, but just accepting {{.}} as the path separator would be 
> sufficient IMO and might be more readable.
+1 to Adrian's suggestion


Repository:
  rL LLVM

https://reviews.llvm.org/D24820



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

Re: [PATCH] D24820: Add -stats-file option

2016-09-23 Thread Matthias Braun via cfe-commits 
MatzeB added inline comments.


Comment at: lib/Driver/Tools.cpp:6102
@@ +6101,3 @@
+StatsFile.assign(Output.getFilename());
+llvm::sys::path::remove_filename(StatsFile);
+  }

bruno wrote:
> Why removing StatsFile here? IIUC, at this point StatsFile is still the same 
> as the output (if it's a file).
a) It behaves like -save-temps=obj (`clang -save-temps=obj foo.c -o bar` will 
give you foo.i, foo.o, ...;
b) This makes sense when you have multiple (`clang -save-stats=obj foo.c bar.c 
-o baz`) inputs for which you need multiple .stats filenames but you only have 
1 output name 
c) It magically works for `-o -` as well



Repository:
  rL LLVM

https://reviews.llvm.org/D24820



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

Re: [PATCH] D24820: Add -stats-file option

2016-09-23 Thread Bruno Cardoso Lopes via cfe-commits 
bruno added inline comments.


Comment at: lib/Driver/Tools.cpp:6102
@@ +6101,3 @@
+StatsFile.assign(Output.getFilename());
+llvm::sys::path::remove_filename(StatsFile);
+  }

Why removing StatsFile here? IIUC, at this point StatsFile is still the same as 
the output (if it's a file).


Repository:
  rL LLVM

https://reviews.llvm.org/D24820



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

Re: [PATCH] D24820: Add -stats-file option

2016-09-22 Thread Adrian Prantl via cfe-commits 
aprantl added inline comments.


Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:111
@@ -110,1 +110,3 @@
+def warn_fe_unable_to_open_stats_file : Warning<
+"unable to open statistic output file '%0': '%1'">;
 def err_fe_no_pch_in_dir : Error<

statstic"s"? not sure.


Comment at: lib/Driver/Tools.cpp:6093
@@ -6092,1 +6092,3 @@
 
+  // Setup statistic file output.
+  if (const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ)) {

ditto :-)


Comment at: test/Driver/save-stats.c:12
@@ +11,3 @@
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c -o 
obj/dir/save-stats.o %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ
+// CHECK-OBJ: "-stats-file=obj/dir{{/|}}save-stats.stats"
+// CHECK-OBJ: "-o" "obj/dir{{/|}}save-stats.o"

This is up to taste, but just accepting {{.}} as the path separator would be 
sufficient IMO and might be more readable.


Repository:
  rL LLVM

https://reviews.llvm.org/D24820



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

[PATCH] D24820: Add -stats-file option

2016-09-21 Thread Matthias Braun via cfe-commits 
MatzeB created this revision.
MatzeB added reviewers: bruno, rsmith, aprantl.
MatzeB added a subscriber: cfe-commits.
MatzeB set the repository for this revision to rL LLVM.
Herald added a subscriber: mcrosier.

This option behaves in a similar spirit as -save-temps and writes out
llvm statistics in json format.

Repository:
  rL LLVM

https://reviews.llvm.org/D24820

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/FrontendOptions.h
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Driver/save-stats.c
  test/Frontend/stats-file.c
  test/Misc/warning-flags.c

Index: test/Misc/warning-flags.c
===
--- test/Misc/warning-flags.c
+++ test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (84):
+CHECK: Warnings without flags (85):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -66,6 +66,7 @@
 CHECK-NEXT:   warn_fe_cc_log_diagnostics_failure
 CHECK-NEXT:   warn_fe_cc_print_header_failure
 CHECK-NEXT:   warn_fe_macro_contains_embedded_newline
+CHECK-NEXT:   warn_fe_unable_to_open_stats_file
 CHECK-NEXT:   warn_file_asm_volatile
 CHECK-NEXT:   warn_ignoring_ftabstop_value
 CHECK-NEXT:   warn_implements_nscopying
Index: test/Frontend/stats-file.c
===
--- /dev/null
+++ test/Frontend/stats-file.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -emit-llvm -o /dev/null -stats-file=%t %s
+// RUN: FileCheck -input-file=%t %s
+// CHECK: {
+//  ... here come some json values ...
+// CHECK: }
+
+// RUN: %clang_cc1 -emit-llvm -o %t -stats-file=%S/doesnotexist/bla %s 2>&1 | FileCheck -check-prefix=OUTPUTFAIL %s
+// OUTPUTFAIL: warning: unable to open statistic output file '{{.*}}{{[/\\]}}doesnotexist{{[/\\]}}bla': '{{[Nn]}}o such file or directory'
Index: test/Driver/save-stats.c
===
--- /dev/null
+++ test/Driver/save-stats.c
@@ -0,0 +1,17 @@
+// RUN: %clang -target x86_64-apple-darwin -save-stats %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin -save-stats=cwd %s -### 2>&1 | FileCheck %s
+// CHECK: "-stats-file=save-stats.stats"
+// CHECK: "{{.*}}save-stats.c"
+
+// RUN: %clang -target x86_64-apple-darwin -S %s -### 2>&1 | FileCheck %s -check-prefix=NO-STATS
+// NO-STATS-NO: -stats-file
+// NO-STATS: "{{.*}}save-stats.c"
+// NO-STATS-NO: -stats-file
+
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c -o obj/dir/save-stats.o %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ
+// CHECK-OBJ: "-stats-file=obj/dir{{/|}}save-stats.stats"
+// CHECK-OBJ: "-o" "obj/dir{{/|}}save-stats.o"
+
+// RUN: %clang -target x86_64-apple-darwin -save-stats=obj -c %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-OBJ-NOO
+// CHECK-OBJ-NOO: "-stats-file=save-stats.stats"
+// CHECK-OBJ-NOO: "-o" "save-stats.o"
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -194,8 +194,10 @@
   }
 
   ~AnalysisConsumer() override {
-if (Opts->PrintStats)
+if (Opts->PrintStats) {
   delete TUTotalTimer;
+  llvm::PrintStatistics();
+}
   }
 
   void DigestAnalyzerOptions() {
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1252,6 +1252,7 @@
   Opts.AuxTriple =
   llvm::Triple::normalize(Args.getLastArgValue(OPT_aux_triple));
   Opts.FindPchSource = Args.getLastArgValue(OPT_find_pch_source_EQ);
+  Opts.StatsFile = Args.getLastArgValue(OPT_stats_file);
 
   if (const Arg *A = Args.getLastArg(OPT_arcmt_check,
  OPT_arcmt_modify,
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -858,7 +858,7 @@
   if (getFrontendOpts().ShowTimers)
 createFrontendTimer();
 
-  if (getFrontendOpts().ShowStats)
+  if (getFrontendOpts().ShowStats || !getFrontendOpts().StatsFile.empty())
 llvm::EnableStatistics();
 
   for (const FrontendInputFile &FIF : getFrontendOpts().Inputs) {
@@ -892,9 +892,24 @@
   OS << " generated.\n";
   }
 
-  if (getFrontendOpts().ShowStats && hasFileManager()) {
-getFileManager().PrintStats();
-OS << "\n";
+  if (getFrontendOpts().ShowStats) {
+if (hasFileManager()) {
+  getFileManager().Pr