[PATCH] D70600: [Error] Add stack traces for llvm::Error invariant violations.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Support/Error.h:762-763
     return std::move(*ValOrErr);
-  else {
-    if (!Msg)
-      Msg = "Failure value returned from cantFail wrapped call";
-#ifndef NDEBUG
-    std::string Str;
-    raw_string_ostream OS(Str);
-    auto E = ValOrErr.takeError();
-    OS << Msg << "\n" << E;
-    Msg = OS.str().c_str();
-#endif
-    llvm_unreachable(Msg);
-  }
+  else
+    detail::fatalCantFailUnhandledError(ValOrErr.takeError(), Msg);
 }
----------------
Drop the else here & similar places ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )


================
Comment at: llvm/lib/Support/Error.cpp:73-74
+  std::lock_guard<std::mutex> Lock(ErrorTraceMutex);
+  if (ErrorTraces->count(&EI))
+    OS << (*ErrorTraces)[&EI];
+  else
----------------
Two map lookups - better to use find & check for end to do only one map lookup.

Actually DenseMap's 'lookup' might suffice.


================
Comment at: llvm/lib/Support/Error.cpp:86-105
+void fatalCantFailUnhandledError(Error Err, const char *Msg) {
+  if (!Msg)
+    Msg = "Failure value returned from cantFail wrapped call";
+  std::string Str;
+  {
+    raw_string_ostream OS(Str);
+    OS << Msg << "\n" << Err << "\n";
----------------
I was going to ask - why all this rather than just asserting and letting the existing crash reporting stuff work. But I guess that crash reporting stuff only works for clang but not other LLVM tools? Perhaps it could be made to work? (though perhaps that's too much work - guess it involves reserving memory for use safely during crashes, etc))

Hmm, I guess it's not entirely about the clang crash reporter - a simple assert failure in llvm-dwarfdump does at least print the stack trace:

  llvm-dwarfdump-tot: /usr/local/google/home/blaikie/dev/llvm/src/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:632: int main(int, char **): Assertion `false' failed.
  Stack dump:
  0.      Program arguments: llvm-dwarfdump-tot -debug-info -v loc.o 
   #0 0x000000000145b977 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:548:11
   #1 0x000000000145bb19 PrintStackTraceSignalHandler(void*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:609: 1

So, yeah - why not just an assert? (or wasn't that what the failure mode was before this change)

Oh, I see it was unreachable and still has an unreachable - that might make this error message a bit redundant - printing the stack trace & then unreachable, which will print the stack trace again?

(the ErrorTraces part is value-add/new functionality here, for sure & assert/unreachable wouldn't ever provide that functionality of course)


================
Comment at: llvm/lib/Support/Error.cpp:175
               "checked prior to being destroyed).\n";
   abort();
 }
----------------
This abort'll also print the stack trace - so seems that the "PrintStackTrace(dbgs())" would be redundant with this as well?


================
Comment at: llvm/lib/Support/InitLLVM.cpp:32-34
+  if (getenv("LLVM_ENABLE_ERROR_TRACING"))
+    enableErrorTracing();
+
----------------
If the code for this is always going to be present - what's the benefit from using a side table rather than making Error itself a little bigger (a pointer) & skip the side table/global state/global ctors/dtors? It's not like storing lots of Errors is a supported scenario, so I doubt an extra pointer in an Error would significantly increase memory usage at any point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70600