[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.


================
Comment at: lldb/source/Core/Communication.cpp:427
   // Notify the read thread.
-  m_connection_sp->InterruptRead();
 
----------------
labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > labath wrote:
> > > > > Have you considered putting this code (some version of it) inside `InterruptRead`? Basically replacing the `select` call inside BytesAvailable with something MainLoop-based ?
> > > > To be honest, I've been considering removing `InterruptRead()` entirely, now that the read loop is triggered by available input rather than read-with-timeout. However, it's still used by editline support.
> > > > 
> > > > That said, I'm not sure what's your idea, given that `Connection` does not have awareness of `Communication` that's using it. I suppose I could pass the `MainLoop` instance as a parameter but we'd still have to maintain a separate version for editline support, and we only have a single caller here.
> > > > To be honest, I've been considering removing `InterruptRead()` entirely, now that the read loop is triggered by available input rather than read-with-timeout. However, it's still used by editline support.
> > > If we could do that, then it would be even better. And it looks like it should be possible to use a MainLoop instance inside the Editline class, instead of the built-in interruption support.
> > > 
> > > > That said, I'm not sure what's your idea, given that `Connection` does not have awareness of `Communication` that's using it. I suppose I could pass the `MainLoop` instance as a parameter but we'd still have to maintain a separate version for editline support, and we only have a single caller here.
> > > 
> > > I don't claim to have thought this out completely, but I was imagining that the MainLoop instance would be internal to the Connection class. The external interface of the Connection class would remain unchanged (so the Communication class would not need to change), and the InterruptRead function would be implemented using the MainLoop functionality.
> > > > That said, I'm not sure what's your idea, given that `Connection` does not have awareness of `Communication` that's using it. I suppose I could pass the `MainLoop` instance as a parameter but we'd still have to maintain a separate version for editline support, and we only have a single caller here.
> > > 
> > > I don't claim to have thought this out completely, but I was imagining that the MainLoop instance would be internal to the Connection class. The external interface of the Connection class would remain unchanged (so the Communication class would not need to change), and the InterruptRead function would be implemented using the MainLoop functionality.
> > 
> > So basically make `ConnectionFileDescriptor::BytesAvailable()` use a main loop to wait for some event, and make `ConnectionFileDescriptor::InterruptRead()` interrupt that?
> > 
> > I suppose both options are feasible but I don't have sufficient foresight to tell which one is better. That said, I have no clue about `ConnectionGenericFileWindows` and I feel that using `MainLoop` would make it possible to reduce code duplication between it and CFD. Though I'd preferred that someone with access to a Windows environment done that.
> > So basically make `ConnectionFileDescriptor::BytesAvailable()` use a main loop to wait for some event, and make `ConnectionFileDescriptor::InterruptRead()` interrupt that?
> 
> Correct.
> 
> > I suppose both options are feasible but I don't have sufficient foresight to tell which one is better. That said, I have no clue about `ConnectionGenericFileWindows` and I feel that using `MainLoop` would make it possible to reduce code duplication between it and CFD. Though I'd preferred that someone with access to a Windows environment done that.
> 
> That is my hope as well, but it's not going to be very easy, since the ReadFile (used by CGFW) api requires you to actually attempt to read some data in order to wait for the result) . That said, I'm not sure this really speaks in favour of one approach over the other. If e.g. we are able to use the same MainLoop code for both CFD and CGFW, then we could probably move that code over to a common base class.
> 
> What I would like to avoid is to increase the number of places that do selects. So, if you can port editline code to something mainloop-based and remove the `InterruptRead`, then that's fine by me, and we can figure out what to do with windows later.
> What I would like to avoid is to increase the number of places that do selects. So, if you can port editline code to something mainloop-based and remove the `InterruptRead`, then that's fine by me, and we can figure out what to do with windows later.

Well, I suppose I'll add that to my TODO and maybe I'll manage to get to it before I retire ;-). Also note that the proposed gdb-remote interrupt changes rely on `InterruptRead()`, so I'd have to change that as well. However, I think it's better to focus on multiprocess first, as that definitely will influence the final shape of how we do async there.


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

https://reviews.llvm.org/D132283