0x1eaf updated this revision to Diff 372510.
0x1eaf added a comment.
addressed review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
https://reviews.llvm.org/D109506
Files:
clang-tools-extra/clangd/JSONTransport.cpp
sammccall added a comment.
Well, thanks for such a well-thought-out patch :-) I have to admit my first
reaction was terror, but this seems solid to me.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1381
+ auto = llvm::errs();
+ OS << "Signalled during AST
0x1eaf added inline comments.
Comment at: clang-tools-extra/clangd/support/ThreadSignalHandler.cpp:22
+ ThreadSignalHandlerCallback *Callback = CurrentCallback.get();
+ // TODO: ignore non thread local signals like SIGTERM
+ if (Callback) {
0x1eaf added a comment.
Thank you for such a prompt and thorough review!
Please consider all the comments I haven't replied to acknowledged and I'm
going to resolve the issues with the next update.
Comment at: clang-tools-extra/clangd/JSONTransport.cpp:117
+ OS <<
sammccall added a comment.
Oops, forgot one thing: you probably want to instrument the preamble-building
in TUScheduler (or in Preamble.cpp?), the background indexing, and code
completion. Those all run the clang parser and should be a rich source of clang
bugs.
Testing idea: `yes '[' | head
sammccall added a comment.
Thanks for doing this, I think this is incredibly useful for debugging.
But also subtle, so please forgive a bunch of comments!
I don't think we're strictly in defined-behavior territory in much of what
these signal handlers are doing, but neither is clang's existing
0x1eaf created this revision.
0x1eaf added reviewers: sammccall, ilya-biryukov, nridge.
0x1eaf added projects: clang, clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman, javed.absar, mgorny.
0x1eaf requested review of this revision.
Herald added subscribers: cfe-commits,
