[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

[Adam Brouwers-Harries via Phabricator via lldb-commits]

aharries-upmem added a comment.

In D93479#2839076 <https://reviews.llvm.org/D93479#2839076>, @jingham wrote:

> I wonder if instead of doing:
>
>   // Use our target to get a shared pointer to ourselves...
>   if (m_finalize_called && !PrivateStateThreadIsValid())
>     BroadcastEvent(event_sp);
>   else
>     m_private_state_broadcaster.BroadcastEvent(event_sp);
>
> ->
>
>   m_private_state_broadcaster.BroadcastEvent(event_sp);
>
> we should have just replaced m_finalize_called with m_finalizing?  If you tried to sent the exited event to the private event broadcaster after it was shut down, that event would never get to the public process event queue.

Hi Jim, I've tried reverting this check and replacing `m_finalize_called` with `m_finalizing`, but sadly the code still seems to deadlock.

I'm also a little suspicious of (in `Process::Finalize`):

  if (m_finalizing.exchange(true))
    return;

rather than

  m_finalize_called = true;

As it would seem (to me) to introduce the possibility of an early exit where there wasn't one before. I don't know if that was intended (to avoid things being done twice?), but it's the only other place I can see a clear change in control flow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479