Refactor TimeProfiler write methods (NFC)

[amonshiz]

Summary

Added a write method for TimeTrace that takes two strings representing
file names. The first is any file name that may have been provided by the
user via time-trace-file flag, and the second is a fallback that should
be configured by the caller. This method makes it cleaner to write the
trace output because there is no longer a need to check file names at the
caller and simplifies future TimeTrace usages.

[rnk]

Once the error handling is fixed up, I'm not sure this is that much better than what came before, but I could be wrong.

llvm/lib/Support/TimeProfiler.cpp

296

Maybe passing the error back to the caller (as the Error type, perhaps?) would be better. LLD would hook it into its diagnostic machinery and clang to its own. Otherwise, the user can supply an unwritable path which will cause clang/lld to print a banner asking them to file a bug report, which is incorrect, it's a usage bug, not a compiler bug.

[amonshiz]

I'm not sure this is that much better than what came before, but I could be wrong.

The benefit of this change is that in the next diff in this stack I add additional usage of the TimeTraceScope to LLVM passes/pass managers. In doing so there is another need to essentially flush the trace info to a file and also validate the file name provided. It made sense to move handling whether or not a user provided a file name to a consistent place.

llvm/lib/Support/TimeProfiler.cpp

296

Will do! Sounds like a good improvement.

[amonshiz]

Add Error based handling

Thanks for the review @rnk

[modocache]

This looks good to me, it sounds like all of @rnk's comments were addressed, and I'd like to unblock D74516 that's stacked on top of this, so I'm going to accept. @amonshiz do you want me to commit this for you? (And @rnk, any comments before I do?)

[amonshiz]

@modocache that would be great if you could land this for me!

lld/ELF/Driver.cpp

case ELF64BEKind:
       link<ELF64BE>(args);
       break;
     default:
       llvm_unreachable("unknown Config->EKind");
     }
   }
 
   if (config->timeTraceEnabled) {
-    // Write the result of the time trace profiler.
-    std::string path = args.getLastArgValue(OPT_time_trace_file_eq).str();
-    if (path.empty())
-      path = (config->outputFile + ".time-trace").str();
-    std::error_code ec;
-    raw_fd_ostream os(path, ec, sys::fs::OF_Text);
-    if (ec) {
-      error("cannot open " + path + ": " + ec.message());
-      return;
-    }
-    timeTraceProfilerWrite(os);
+    timeTraceProfilerWrite(args.getLastArgValue(OPT_time_trace_file_eq).str(),
+                           config->outputFile);
     timeTraceProfilerCleanup();
   }
 }
 
 static std::string getRpath(opt::InputArgList &args) {
   std::vector<StringRef> v = args::getStrings(args, OPT_rpath);
   return llvm::join(v.begin(), v.end(), ":");
 }

llvm/include/llvm/Support/TimeProfiler.h

 /// Finish a time trace profiler running on a worker thread.
 void timeTraceProfilerFinishThread();
 
 /// Is the time trace profiler enabled, i.e. initialized?
 inline bool timeTraceProfilerEnabled() {
   return getTimeTraceProfilerInstance() != nullptr;
 }
 
-/// Write profiling data to output file.
+/// Write profiling data to output stream.
 /// Data produced is JSON, in Chrome "Trace Event" format, see
 /// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
 void timeTraceProfilerWrite(raw_pwrite_stream &OS);
 
+/// Write profiling data to a file.
+/// The function will write to \p PreferredFileName if provided, if not
+/// then will write to \p FallbackFileName appending .time-trace.
+/// Triggers a fatal error if a file cannot be opened for writing.
+void timeTraceProfilerWrite(StringRef PreferredFileName,
+                            StringRef FallbackFileName);
+
 /// Manually begin a time section, with the given \p Name and \p Detail.
 /// Profiler copies the string data, so the pointers can be given into
 /// temporaries. Time sections can be hierarchical; every Begin must have a
 /// matching End pair but they can nest.
 void timeTraceProfilerBegin(StringRef Name, StringRef Detail);
 void timeTraceProfilerBegin(StringRef Name,
                             llvm::function_ref<std::string()> Detail);
 

llvm/lib/Support/TimeProfiler.cpp

 }
 
 void timeTraceProfilerWrite(raw_pwrite_stream &OS) {
   assert(TimeTraceProfilerInstance != nullptr &&
          "Profiler object can't be null");
   TimeTraceProfilerInstance->Write(OS);
 }
 
+void timeTraceProfilerWrite(StringRef PreferredFileName,
+                            StringRef FallbackFileName) {
+  assert(TimeTraceProfilerInstance != nullptr &&
+         "Profiler object can't be null");
+
+  std::string Path = PreferredFileName.str();
+  if (Path.empty()) {
+    Path = FallbackFileName == "-" ? "out" : FallbackFileName.str();
+    Path += ".time-trace";
+  }
+
+  std::error_code EC;
+  raw_fd_ostream OS(Path, EC, sys::fs::OF_Text);
+  if (EC)
+    report_fatal_error("Could not open " + Path + ": " + EC.message());
+
+  timeTraceProfilerWrite(OS);
+}
+
 void timeTraceProfilerBegin(StringRef Name, StringRef Detail) {
   if (TimeTraceProfilerInstance != nullptr)
     TimeTraceProfilerInstance->begin(std::string(Name),
                                      [&]() { return std::string(Detail); });
 }
 
 void timeTraceProfilerBegin(StringRef Name,
                             llvm::function_ref<std::string()> Detail) {