[PATCH] D70600: [Error] Add stack traces for llvm::Error invariant violations.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 22 17:50:25 PST 2019
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
More information about the llvm-commits
mailing list