[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Other than the lifetime issue, I think this is basically ready.



================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2014
+  llvm::TimeTraceScope TimeScope("InstantiateClass", [&]() {
+      return Instantiation->getQualifiedNameAsString().c_str();
+    });
----------------
Shoot, I think the callable should probably return std::string instead of StringRef, otherwise this looks like it will be a use-after-free. You allocate a temporary std::string, get a pointer to the characters, return, the string is destroyed, and then UAF.

With a std::string return type, you won't need `.c_str()`.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4089
+  llvm::TimeTraceScope TimeScope("InstantiateFunction", [&]() {
+      return Function->getQualifiedNameAsString().c_str();
+    });
----------------
`.c_str()` is unnecessary


================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:54-58
+  TimeTraceScope(const char *Name, const char *Detail) {
+    if (TimeTraceProfilerInstance != nullptr)
+      timeTraceProfilerBegin(Name, Detail);
+  }
+  TimeTraceScope(const char *Name, llvm::function_ref<StringRef()> Detail) {
----------------
I think you can replace `const char *` in these prototypes with StringRef to save a few `.data()` calls at some call sites.

As mentioned before, I think the callable needs to return `std::string`, though.


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

https://reviews.llvm.org/D58675