[PATCH] D78903: [Driver] Add option -fproc-stat-report

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 2 inline comments as done.
sepavloff added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3782
+          = Cmd.getProcessStatistics();
+      if (ProcStat) {
+        if (PrintProcessStat) {
----------------
aganea wrote:
> In the case where `!ProcStat`, I am wondering if we shouldn't emit zero values, in the report file at least. Otherwise if an invocation fails, it won't be there in the report and we might wonder why. Emitting 0 might indicate that something went wrong.
The feature allows getting statistics using conventional tools like `make`. In this case the absence of numbers for a particular file is not an issue, maybe the file has already been built. If something goes wrong, clang must handle it in `Compilation::ExecuteCommand` and react properly. The fact of failed invocation would be handled by build tool. For the purpose of monitoring performance numbers it is more convenient to record successful compilations only.





================
Comment at: clang/lib/Driver/Driver.cpp:3814
+          llvm::raw_fd_ostream OS(StatReportFile, EC, llvm::sys::fs::OF_Append);
+          if (!EC) {
+            if (auto L = OS.tryToLock())
----------------
aganea wrote:
> If the goal is to report accurate information, maybe it's worth looping here a bit in case of an error, to give the chance to other clang.exe instances to release the file lock? What do you think? (same for `tryToLock`)
Actually this method makes blocking request and waits infinitely.

There is long discussion of how this lock helper must be implemented: https://reviews.llvm.org/D79066. Finally the variant with timeout was removed, only blocking one remained. The method has prefix `try` because it requires handling result value.

If you think the name or interface might be better, you can participate in the discussion in D79066, that patch isn't committed yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903