[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.

Hi all,

Apologies for being the bearer of bad news, but I believe that this patch 
breaks our[1] (downstream) lldb, by introducing a deadlock when a process is 
killed by a parent debugging process. Specifically, I believe that this patch 
causes a process to fail to exit, which causes a later deadlock when a listener 
waits for the process to exit.

I'm in the process of trying to produce some convincing output from our backend 
so that I can properly file a bug, but in the meantime I wanted to raise this 
here in case anyone has any comments or thoughts on this. Although I managed to 
narrow the issue down to this commit through a `git bisect`, I am not confident 
in this patch being the *cause* of the deadlock, as it may be the case that we 
are relying on incorrect behaviour in lldb that this patch fixes.

[1]: https://github.com/upmem/llvm-project/tree/upmem_release_120


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 , @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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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#2840497 , @aharries-upmem 
wrote:

> I'm also a little suspicious of (in `Process::Finalize`):
>
>   if (m_finalizing.exchange(true))
> return;
>
> rather than
>
>   m_finalize_called = true;

I've also tried replacing this with `m_finalizing.exchange(true);`, which also 
(sadly) doesn't seem to stop the deadlock.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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#2841049 , @jingham wrote:

> Huh...  I fear you are going to have to debug this further on your end.  I 
> can't see anything else suspect in this patch.

Agreed. Sadly I need to prioritise another task in the short term, but I think 
I'm going to try slowly re-implementing the changes in the patch to see I can 
find a minimal change that triggers it when I have time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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#2850445 , @jingham wrote:
>  Don't hesitate to reach out for questions, explanations, "what the heck"-s 
> once you get back to this.

Thanks Jim, that's very kind of you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits