[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

Wed Oct 30 16:17:00 PDT 2019

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

Thanks for identifying this race condition.

After some poking around, I think there's a cleaner way to address it.

First, `m_session_state`'s lifetime is currently managed mostly by `ProcessDebugger` (base class) but for one exception in `ProcessWindows` (derived class).  There doesn't seem to be a good reason for that inconsistency, so my first step was to eliminate it and make `ProcessDebugger` responsible for its lifetime in all cases. This is done by moving the `m_session_state.reset()` to `ProcessDebugger::OnExitProcess` and having `ProcessWindows::OnExitProcess` call the other.  This has the nice additional benefit of eliminating some duplicate code and comments.

But I'm not sure we need to clear `m_session_state` even in `ProcessDebugger::OnExitProcess`.  There are two places where `m_session_state` is created:  `ProcessDebugger::AttachProcess` and `ProcessDebugger::LaunchProcess`.  We could put `m_session_state.reset()`s in those functions, where it handles failure of `WaitForDebuggerConnection`.  But I'm not even sure if _that_ is necessary.

By adding a virtual destructor to the base class, it seems everything cleans up naturally before starting the next debugging session.

Here's what it would look like:  https://reviews.llvm.org/P8168

(If I'm wrong, and it _is_ important for us to clear the session state explicitly, then we'd need to find a place after `OnExitProcess`.  I don't think such a hook exists right now, so perhaps we'd have to create one.  That hook could be the one place we reset `m_session_state` for both the successful and unsuccessful debug sessions.)

FYI:  I'll be offline until November 4.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69503