[PATCH] D63750: [WIP} Dump current PrettyStackTrace on SIGINFO (Ctrl-T)

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: lib/Support/PrettyStackTrace.cpp:52
+// practice the worst that will happen is that we won't print a stack trace when
+// we could have.
+static volatile std::atomic<unsigned> GlobalSigInfoGenerationCounter{};
----------------
Why not use `seq_cst` everywhere? It's not in any critical path, the the small extra cost isn't relevant. I'm also not sure that you really need `volatile` here: `AddSignalHandler` also synchronizes so a sufficiently smart optimizer won't be able to prove that anything can be aggregated.


================
Comment at: lib/Support/PrettyStackTrace.cpp:53
+// we could have.
+static volatile std::atomic<unsigned> GlobalSigInfoGenerationCounter{};
+static LLVM_THREAD_LOCAL unsigned ThreadLocalSigInfoGenerationCounter = 0;
----------------
Technically, in C++11, you need to initialize `std::atomic` with `ATOMIC_VAR_INIT`.


================
Comment at: lib/Support/Unix/Signals.inc:231
 
+/// Signals that represent requests for status
+static const int InfoSigs[] = {
----------------
Period at end of sentence


================
Comment at: lib/Support/Unix/Signals.inc:296
 
-  auto registerHandler = [&](int Signal) {
+  auto registerHandler = [&](int Signal, bool IsKillSignal) {
     unsigned Index = NumRegisteredSignals.load();
----------------
I'd rather have `enum class SignalKind { IsKill, IsInfo };` instead of a bool parameter.


================
Comment at: lib/Support/Windows/Signals.inc:561
+void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
+  // Unimplemented.
+}
----------------
wai?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63750