[Lldb-commits] [lldb] SBDebugger: Add new APIs `AddDestroyCallback` and `RemoveDestroyCallback` (PR #89868)

[via lldb-commits]

jimingham wrote:



> On May 7, 2024, at 2:51 PM, royitaqi ***@***.***> wrote:
> 
> 
> @royitaqi commented on this pull request.
> 
> In lldb/include/lldb/Core/Debugger.h <https://github.com/llvm/llvm-project/pull/89868#discussion_r1593145799>:
> 
> > @@ -731,8 +747,11 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
>    lldb::TargetSP m_dummy_target_sp;
>    Diagnostics::CallbackID m_diagnostics_callback_id;
>  
> -  lldb_private::DebuggerDestroyCallback m_destroy_callback = nullptr;
> -  void *m_destroy_callback_baton = nullptr;
> +  std::recursive_mutex m_destroy_callback_mutex;
> BTW:
> 
> I think you need to at least protect adding.
> 
> To me, it's either none, or all.
> 
IME you don't want to just keep adding mutexes willy-nilly, you do want to know that the resource could be contended for by operations on more than one thread. My only point was that we couldn't think of a plausible reason to lock when iterating for destroy, but it's easy to see that you need to lock adding and removing since there's nothing that keeps that from happening.

> If we decide to protect adding (multiple adds from different threads not during destroy), then we should just protect all of add/remove/destroy, because any of these operations can happen at the same time from multiple threads.
> 
> FWIW I agree with what @JDevlieghere <https://github.com/JDevlieghere> said:
> 
> I'd say let's not overthink this and if we want to change that, let's do it holistically for the whole class instead of piecemeal.
> 
I didn't really understand that statement.  Adding debuggers, or querying them is controlled by the debugger list mutex.  Manipulating the I/O handlers is governed by the io handler stack mutex, there's a lock around the interrupt requests.  Debugger doesn't seem like a class that is not locking the lists of things it controls as a general rule.

When you have a collection that can be accessed from multiple threads, adding and removing, then you really do need to protect those accesses.  I'm not sure it's good reasoning to say "That's not likely, let's wait till it crashes on someone to add the protection".

Jim

> —
> Reply to this email directly, view it on GitHub <https://github.com/llvm/llvm-project/pull/89868#discussion_r1593145799>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZU2E5VV5VVISC4TFTZBFEFHAVCNFSM6AAAAABGWDO5TCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBUGI2TSMBXGM>.
> You are receiving this because you were mentioned.
> 



https://github.com/llvm/llvm-project/pull/89868