[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa845aeb5d6c8: [Driver] Allow to collect `-save-stats` data 
to a file specified in the… (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

Files:
  clang/docs/CommandGuide/clang.rst
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Driver/save-stats.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -307,6 +307,9 @@
   TheDriver.CCPrintProcessStats =
   checkEnvVar("CC_PRINT_PROC_STAT", "CC_PRINT_PROC_STAT_FILE",
 TheDriver.CCPrintStatReportFilename);
+  TheDriver.CCPrintInternalStats =
+  checkEnvVar("CC_PRINT_INTERNAL_STAT", "CC_PRINT_INTERNAL_STAT_FILE",
+TheDriver.CCPrintInternalStatReportFilename);
 
   return true;
 }
Index: clang/test/Driver/save-stats.c
===
--- clang/test/Driver/save-stats.c
+++ clang/test/Driver/save-stats.c
@@ -26,3 +26,14 @@
 
 // RUN: %clang -target x86_64-linux-unknown -save-stats=obj -flto -o obj/dir/save-stats.exe %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-LTO-OBJ
 // CHECK-LTO-OBJ: "-plugin-opt=stats-file=obj/dir{{/|}}save-stats.stats"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV
+// CHECK-ENV: "-stats-file=-"
+// CHECK-ENV-NO: "stats-file-append"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: CC_PRINT_INTERNAL_STAT_FILE=/tmp/stats.json \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV-FILE
+// CHECK-ENV-FILE: "-stats-file=/tmp/stats.json"
+// CHECK-ENV-FILE: "-stats-file-append"
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1087,9 +1087,12 @@
   }
   StringRef StatsFile = getFrontendOpts().StatsFile;
   if (!StatsFile.empty()) {
+llvm::sys::fs::OpenFlags FileFlags = llvm::sys::fs::OF_TextWithCRLF;
+if (getFrontendOpts().AppendStats)
+  FileFlags |= llvm::sys::fs::OF_Append;
 std::error_code EC;
-auto StatS = std::make_unique(
-StatsFile, EC, llvm::sys::fs::OF_TextWithCRLF);
+auto StatS =
+std::make_unique(StatsFile, EC, FileFlags);
 if (EC) {
   getDiagnostics().Report(diag::warn_fe_unable_to_open_stats_file)
   << StatsFile << EC.message();
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1766,22 +1766,29 @@
  const InputInfo ,
  const Driver ) {
   const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
-  if (!A)
+  if (!A && !D.CCPrintInternalStats)
 return {};
 
-  StringRef SaveStats = A->getValue();
   SmallString<128> StatsFile;
-  if (SaveStats == "obj" && Output.isFilename()) {
-StatsFile.assign(Output.getFilename());
-llvm::sys::path::remove_filename(StatsFile);
-  } else if (SaveStats != "cwd") {
-D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
-return {};
-  }
+  if (A) {
+StringRef SaveStats = A->getValue();
+if (SaveStats == "obj" && Output.isFilename()) {
+  StatsFile.assign(Output.getFilename());
+  llvm::sys::path::remove_filename(StatsFile);
+} else if (SaveStats != "cwd") {
+  D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+  return {};
+}
 
-  StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-  llvm::sys::path::append(StatsFile, BaseName);
-  llvm::sys::path::replace_extension(StatsFile, "stats");
+StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
+llvm::sys::path::append(StatsFile, BaseName);
+llvm::sys::path::replace_extension(StatsFile, "stats");
+  } else {
+assert(D.CCPrintInternalStats);
+StatsFile.assign(D.CCPrintInternalStatReportFilename.empty()
+ ? "-"
+ : D.CCPrintInternalStatReportFilename);
+  }
   return StatsFile;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- 

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Akira Hatanaka via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Adding folks involved with LTO. The problem is that stats emitted during the 
first round of compilation are likely to be overwritten by stats emitted during 
[Thin]LTO itself. Not touching it now to preserve the existing behavior. But 
wanted to inform you about this shortcoming in case it is important for you to 
fix.

Original change adding `-save-stats` for LTO is D45531 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 503488.
vsapsai added a comment.

Don't touch LTO pipeline to avoid breaking existing workflows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

Files:
  clang/docs/CommandGuide/clang.rst
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Driver/save-stats.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -307,6 +307,9 @@
   TheDriver.CCPrintProcessStats =
   checkEnvVar("CC_PRINT_PROC_STAT", "CC_PRINT_PROC_STAT_FILE",
 TheDriver.CCPrintStatReportFilename);
+  TheDriver.CCPrintInternalStats =
+  checkEnvVar("CC_PRINT_INTERNAL_STAT", "CC_PRINT_INTERNAL_STAT_FILE",
+TheDriver.CCPrintInternalStatReportFilename);
 
   return true;
 }
Index: clang/test/Driver/save-stats.c
===
--- clang/test/Driver/save-stats.c
+++ clang/test/Driver/save-stats.c
@@ -26,3 +26,14 @@
 
 // RUN: %clang -target x86_64-linux-unknown -save-stats=obj -flto -o obj/dir/save-stats.exe %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-LTO-OBJ
 // CHECK-LTO-OBJ: "-plugin-opt=stats-file=obj/dir{{/|}}save-stats.stats"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV
+// CHECK-ENV: "-stats-file=-"
+// CHECK-ENV-NO: "stats-file-append"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: CC_PRINT_INTERNAL_STAT_FILE=/tmp/stats.json \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV-FILE
+// CHECK-ENV-FILE: "-stats-file=/tmp/stats.json"
+// CHECK-ENV-FILE: "-stats-file-append"
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1087,9 +1087,12 @@
   }
   StringRef StatsFile = getFrontendOpts().StatsFile;
   if (!StatsFile.empty()) {
+llvm::sys::fs::OpenFlags FileFlags = llvm::sys::fs::OF_TextWithCRLF;
+if (getFrontendOpts().AppendStats)
+  FileFlags |= llvm::sys::fs::OF_Append;
 std::error_code EC;
-auto StatS = std::make_unique(
-StatsFile, EC, llvm::sys::fs::OF_TextWithCRLF);
+auto StatS =
+std::make_unique(StatsFile, EC, FileFlags);
 if (EC) {
   getDiagnostics().Report(diag::warn_fe_unable_to_open_stats_file)
   << StatsFile << EC.message();
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1785,22 +1785,29 @@
  const InputInfo ,
  const Driver ) {
   const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
-  if (!A)
+  if (!A && !D.CCPrintInternalStats)
 return {};
 
-  StringRef SaveStats = A->getValue();
   SmallString<128> StatsFile;
-  if (SaveStats == "obj" && Output.isFilename()) {
-StatsFile.assign(Output.getFilename());
-llvm::sys::path::remove_filename(StatsFile);
-  } else if (SaveStats != "cwd") {
-D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
-return {};
-  }
+  if (A) {
+StringRef SaveStats = A->getValue();
+if (SaveStats == "obj" && Output.isFilename()) {
+  StatsFile.assign(Output.getFilename());
+  llvm::sys::path::remove_filename(StatsFile);
+} else if (SaveStats != "cwd") {
+  D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+  return {};
+}
 
-  StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-  llvm::sys::path::append(StatsFile, BaseName);
-  llvm::sys::path::replace_extension(StatsFile, "stats");
+StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
+llvm::sys::path::append(StatsFile, BaseName);
+llvm::sys::path::replace_extension(StatsFile, "stats");
+  } else {
+assert(D.CCPrintInternalStats);
+StatsFile.assign(D.CCPrintInternalStatReportFilename.empty()
+ ? "-"
+ : D.CCPrintInternalStatReportFilename);
+  }
   return StatsFile;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7034,8 +7034,11 @@
 
   // Setup statistics 

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/test/Driver/save-stats.c:26
 // CHECK-LTO: "-plugin-opt=stats-file=save-stats.stats"
+// CHECK-LTO: "-plugin-opt=-stats-file-append"
 

ahatanak wrote:
> ahatanak wrote:
> > Doesn't this require `stats-file-append` to be supported by the plugin just 
> > like `stats-file` is supported?
> > 
> > https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L309
> Do we want to pass `stats-file-append` in `tools::addLTOOptions`?
You are right with the gold-plugin. I think I'll drop any changes for LTO now 
because that's not my purpose and I don't want to change emission of stats for 
something I don't work on. Also I believe it creates discrepancies for 
different linker plugins and I really don't want to introduce this 
inconsistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/test/Driver/save-stats.c:26
 // CHECK-LTO: "-plugin-opt=stats-file=save-stats.stats"
+// CHECK-LTO: "-plugin-opt=-stats-file-append"
 

ahatanak wrote:
> Doesn't this require `stats-file-append` to be supported by the plugin just 
> like `stats-file` is supported?
> 
> https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L309
Do we want to pass `stats-file-append` in `tools::addLTOOptions`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/test/Driver/save-stats.c:26
 // CHECK-LTO: "-plugin-opt=stats-file=save-stats.stats"
+// CHECK-LTO: "-plugin-opt=-stats-file-append"
 

Doesn't this require `stats-file-append` to be supported by the plugin just 
like `stats-file` is supported?

https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L309


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 501709.
vsapsai added a comment.

Append to stats file when using `CC_PRINT_INTERNAL_STAT` and for LTO when 
reusing the same stats file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

Files:
  clang/docs/CommandGuide/clang.rst
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Driver/save-stats.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -307,6 +307,9 @@
   TheDriver.CCPrintProcessStats =
   checkEnvVar("CC_PRINT_PROC_STAT", "CC_PRINT_PROC_STAT_FILE",
 TheDriver.CCPrintStatReportFilename);
+  TheDriver.CCPrintInternalStats =
+  checkEnvVar("CC_PRINT_INTERNAL_STAT", "CC_PRINT_INTERNAL_STAT_FILE",
+TheDriver.CCPrintInternalStatReportFilename);
 
   return true;
 }
Index: clang/test/Driver/save-stats.c
===
--- clang/test/Driver/save-stats.c
+++ clang/test/Driver/save-stats.c
@@ -23,6 +23,19 @@
 // CHECK-LTO: "-stats-file=save-stats.stats"
 // CHECK-LTO: "-o" "obj/dir{{/|}}save-stats.exe"
 // CHECK-LTO: "-plugin-opt=stats-file=save-stats.stats"
+// CHECK-LTO: "-plugin-opt=-stats-file-append"
 
 // RUN: %clang -target x86_64-linux-unknown -save-stats=obj -flto -o obj/dir/save-stats.exe %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-LTO-OBJ
 // CHECK-LTO-OBJ: "-plugin-opt=stats-file=obj/dir{{/|}}save-stats.stats"
+// CHECK-LTO-OBJ: "-plugin-opt=-stats-file-append"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV
+// CHECK-ENV: "-stats-file=-"
+// CHECK-ENV-NO: "stats-file-append"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: CC_PRINT_INTERNAL_STAT_FILE=/tmp/stats.json \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV-FILE
+// CHECK-ENV-FILE: "-stats-file=/tmp/stats.json"
+// CHECK-ENV-FILE: "-stats-file-append"
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1087,9 +1087,12 @@
   }
   StringRef StatsFile = getFrontendOpts().StatsFile;
   if (!StatsFile.empty()) {
+llvm::sys::fs::OpenFlags FileFlags = llvm::sys::fs::OF_TextWithCRLF;
+if (getFrontendOpts().AppendStats)
+  FileFlags |= llvm::sys::fs::OF_Append;
 std::error_code EC;
-auto StatS = std::make_unique(
-StatsFile, EC, llvm::sys::fs::OF_TextWithCRLF);
+auto StatS =
+std::make_unique(StatsFile, EC, FileFlags);
 if (EC) {
   getDiagnostics().Report(diag::warn_fe_unable_to_open_stats_file)
   << StatsFile << EC.message();
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -768,9 +768,13 @@
 
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
-  if (!StatsFile.empty())
+  if (!StatsFile.empty()) {
 CmdArgs.push_back(
 Args.MakeArgString(Twine(PluginOptPrefix) + "stats-file=" + StatsFile));
+// Don't overwrite the same stats file but append to it.
+CmdArgs.push_back(
+Args.MakeArgString(Twine(PluginOptPrefix) + "-stats-file-append"));
+  }
 
   // Setup crash diagnostics dir.
   if (Arg *A = Args.getLastArg(options::OPT_fcrash_diagnostics_dir))
@@ -1785,22 +1789,29 @@
  const InputInfo ,
  const Driver ) {
   const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
-  if (!A)
+  if (!A && !D.CCPrintInternalStats)
 return {};
 
-  StringRef SaveStats = A->getValue();
   SmallString<128> StatsFile;
-  if (SaveStats == "obj" && Output.isFilename()) {
-StatsFile.assign(Output.getFilename());
-llvm::sys::path::remove_filename(StatsFile);
-  } else if (SaveStats != "cwd") {
-D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
-return {};
-  }
+  if (A) {
+StringRef SaveStats = A->getValue();
+if (SaveStats == "obj" && Output.isFilename()) {
+  StatsFile.assign(Output.getFilename());
+  llvm::sys::path::remove_filename(StatsFile);
+} else if (SaveStats != "cwd") {
+  D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+  return {};

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added inline comments.



Comment at: clang/lib/Frontend/CompilerInstance.cpp:1093
+StatsFile, EC,
+llvm::sys::fs::OF_Append | llvm::sys::fs::OF_TextWithCRLF);
 if (EC) {

ahatanak wrote:
> `OF_Append` is necessary when the results are written to a single file 
> specified via `CC_PRINT_INTERNAL_STAT_FILE`, but do we want this change for 
> users passing `-save-stats` to the command line?
For implicit modules appending seems to be correct and desirable as we store 
information about all CompilerInstance and not only for the last one. But as 
`llvm-test-suite` 
https://github.com/llvm/llvm-test-suite/blob/main/litsupport/modules/stats.py 
doesn't expect appended stats, I'll look into doing it only for 
environment-driven stats collection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/lib/Frontend/CompilerInstance.cpp:1093
+StatsFile, EC,
+llvm::sys::fs::OF_Append | llvm::sys::fs::OF_TextWithCRLF);
 if (EC) {

`OF_Append` is necessary when the results are written to a single file 
specified via `CC_PRINT_INTERNAL_STAT_FILE`, but do we want this change for 
users passing `-save-stats` to the command line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: MatzeB, ahatanak.
Herald added a subscriber: ributzka.
Herald added a project: All.
vsapsai requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Using two environment variables `CC_PRINT_INTERNAL_STAT` and
`CC_PRINT_INTERNAL_STAT_FILE` to work like `CC_PRINT_PROC_STAT`.

The purpose of the change is to allow collecting the internal stats
without modifying the build scripts. Write all stats to a single file
to simplify aggregating the data.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144981

Files:
  clang/docs/CommandGuide/clang.rst
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Driver/save-stats.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -307,6 +307,9 @@
   TheDriver.CCPrintProcessStats =
   checkEnvVar("CC_PRINT_PROC_STAT", "CC_PRINT_PROC_STAT_FILE",
 TheDriver.CCPrintStatReportFilename);
+  TheDriver.CCPrintInternalStats =
+  checkEnvVar("CC_PRINT_INTERNAL_STAT", "CC_PRINT_INTERNAL_STAT_FILE",
+TheDriver.CCPrintInternalStatReportFilename);
 
   return true;
 }
Index: clang/test/Driver/save-stats.c
===
--- clang/test/Driver/save-stats.c
+++ clang/test/Driver/save-stats.c
@@ -26,3 +26,12 @@
 
 // RUN: %clang -target x86_64-linux-unknown -save-stats=obj -flto -o obj/dir/save-stats.exe %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-LTO-OBJ
 // CHECK-LTO-OBJ: "-plugin-opt=stats-file=obj/dir{{/|}}save-stats.stats"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV
+// CHECK-ENV: "-stats-file=-"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: CC_PRINT_INTERNAL_STAT_FILE=/tmp/stats.json \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV-FILE
+// CHECK-ENV-FILE: "-stats-file=/tmp/stats.json"
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1089,7 +1089,8 @@
   if (!StatsFile.empty()) {
 std::error_code EC;
 auto StatS = std::make_unique(
-StatsFile, EC, llvm::sys::fs::OF_TextWithCRLF);
+StatsFile, EC,
+llvm::sys::fs::OF_Append | llvm::sys::fs::OF_TextWithCRLF);
 if (EC) {
   getDiagnostics().Report(diag::warn_fe_unable_to_open_stats_file)
   << StatsFile << EC.message();
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1785,22 +1785,29 @@
  const InputInfo ,
  const Driver ) {
   const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
-  if (!A)
+  if (!A && !D.CCPrintInternalStats)
 return {};
 
-  StringRef SaveStats = A->getValue();
   SmallString<128> StatsFile;
-  if (SaveStats == "obj" && Output.isFilename()) {
-StatsFile.assign(Output.getFilename());
-llvm::sys::path::remove_filename(StatsFile);
-  } else if (SaveStats != "cwd") {
-D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
-return {};
-  }
+  if (A) {
+StringRef SaveStats = A->getValue();
+if (SaveStats == "obj" && Output.isFilename()) {
+  StatsFile.assign(Output.getFilename());
+  llvm::sys::path::remove_filename(StatsFile);
+} else if (SaveStats != "cwd") {
+  D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+  return {};
+}
 
-  StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-  llvm::sys::path::append(StatsFile, BaseName);
-  llvm::sys::path::replace_extension(StatsFile, "stats");
+StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
+llvm::sys::path::append(StatsFile, BaseName);
+llvm::sys::path::replace_extension(StatsFile, "stats");
+  } else {
+assert(D.CCPrintInternalStats);
+StatsFile.assign(D.CCPrintInternalStatReportFilename.empty()
+ ? "-"
+ : D.CCPrintInternalStatReportFilename);
+  }
   return StatsFile;
 }
 
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -199,8 +199,9 @@
   ClangExecutable(ClangExecutable), SysRoot(DEFAULT_SYSROOT),
   DriverTitle(Title),