Page MenuHomePhabricator

[clangd] Remove no-op crash handler, we never set a crash context.
ClosedPublic

Authored by sammccall on Oct 9 2018, 12:04 PM.

Details

Summary

I think this was just copied from somewhere with the belief that it actually
did some crash handling.

Of course the question arises: *should* we set one? I don't think so:

  • clangd used to crash a lot, now it's pretty stable, because we found and fixed the crashes. I think the long-term effects of crashing hard are good.
  • the implementation can't do any magic, it just uses longjmp to return without running any destructors by default. This is unsafe in general (e.g. mutexes won't unlock) and will certainly end up leaking memory. Whatever UB caused the crash may still stomp all over global state, etc.

I think there's an argument for isolating the background indexer (autoindex)
because it's not directly under the user's control, the crash surface is larger,
and it doesn't particularly need to interact with the rest of clangd.
But there, fork() and communicate through the FS is safer.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Oct 9 2018, 12:04 PM
ioeric accepted this revision.Oct 11 2018, 4:30 AM

LGTM

Of course the question arises: *should* we set one

Agree with your points. If the cleanup is unsafe, it might end up biting us harder in the future.

This revision is now accepted and ready to land.Oct 11 2018, 4:30 AM
This revision was automatically updated to reflect the committed changes.