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

[Lang Hames via Phabricator via llvm-commits]

lhames marked 5 inline comments as done.
lhames 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);
 }
----------------
dblaikie wrote:
> Drop the else here & similar places ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )
Yep.


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


================
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";
----------------
dblaikie wrote:
> 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)
> I was going to ask - why all this rather than just asserting and letting the existing crash reporting stuff work.

The benefit to printing ourselves is that we can disambiguate the stack traces ("this is for the error, this is for where you dropped it") and ensure (by calling PrintStackTrace manually) that these two things are rendered side-by-side. I have not satisfied myself that assertions will always generate a similar trace (and not be intercepted by some crash logging tool) yet, but if that's the case then we can just do that.


================
Comment at: llvm/lib/Support/Error.cpp:175
               "checked prior to being destroyed).\n";
   abort();
 }
----------------
dblaikie wrote:
> This abort'll also print the stack trace - so seems that the "PrintStackTrace(dbgs())" would be redundant with this as well?
Yep -- error message polish is to-do here. We do want to avoid redundant traces.


================
Comment at: llvm/lib/Support/InitLLVM.cpp:32-34
+  if (getenv("LLVM_ENABLE_ERROR_TRACING"))
+    enableErrorTracing();
+
----------------
dblaikie wrote:
> 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?
It's sort of much-of-a-muchness. We do support multiple in-flight errors, so I like the idea of not increasing the size of ErrorInfo for something that won't be used 99.9% of the time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70600