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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3782
+          = Cmd.getProcessStatistics();
+      if (ProcStat) {
+        if (PrintProcessStat) {
----------------
sepavloff wrote:
> 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.
> 
> 
> 
Ok, thanks for clarifying! Could you please do an early exit here? ie.
```
if (!ProcStat)
  return;
```


================
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())
----------------
sepavloff wrote:
> 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.
Thanks!

In the same way as above, I think the code will read better if it did early exits. Also, I don't think you need `handleAllErrors`, you could replace it with `toString` and report the error code the user:
```
if (EC)
  return;
auto L=OS.tryToLock();
if (!L) {
  llvm::errs() ... << toString(L.takeError());
  return;
}
OS << Buffer;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903