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

[Jordan Rose via Phabricator via llvm-commits]

jordan_rose marked an inline comment as done.
jordan_rose 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{};
----------------
jfb wrote:
> 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.
The store could be `seq_cst`, but I wanted the load to be unsychronized because I never want people thinking that PrettyStackTraces are expensive. It's also not `AddSignalHandler` we're doing here, but a bare setting of the signal function, which is the classic use for `volatile`.


================
Comment at: lib/Support/Windows/Signals.inc:561
+void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
+  // Unimplemented.
+}
----------------
jfb wrote:
> wai?
Copying my justification about "there doesn't seem to be a SIGINFO equivalent on Windows" seemed…not great, since it might dissuade people from correcting Past Me. I can add it if you think it's an improvement, though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63750