This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Core] Use thread for Communication::Write() as well
Needs ReviewPublic

Authored by mgorny on Aug 24 2022, 11:06 AM.

Details

Summary

When read thread is enabled, perform all the writes in the thread
as well. Most importantly, this ensures that we do not attempt to
simultaneously read and write the same fd from different threads.

Sponsored by: The FreeBSD Foundation

Diff Detail

Event Timeline

mgorny created this revision.Aug 24 2022, 11:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 11:06 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mgorny updated this revision to Diff 455295.Aug 24 2022, 11:10 AM

Update to the version with error support.

Can you explain where you're heading with this? I can imagine that something like this might be necessary in some cases, but I also don't see anything inherently wrong with concurrent reads and writes to the same connection.

Well, back when working on the async thread, you've indicated that it's a bad idea to read from one thread while writing from another, so I've basically focused on getting to the point when all reads and writes are guaranteed to happen from a single thread.

(with the final goal of using StartReadThread() inside gdb-remote comms to take care of reading the data for concurrent process plugin instances)

Well, back when working on the async thread, you've indicated that it's a bad idea to read from one thread while writing from another, so I've basically focused on getting to the point when all reads and writes are guaranteed to happen from a single thread.

Ok, I think I know what you mean. That comment was made in the context of the GDB communication class, whose functionality is more complex (sending a packet involves both reading and writing) than the plain Communication. Sending packets from multiple threads is more complex that a hypothetical simple setup where you have a single thread just doing reading, and another thread just for writing...

(with the final goal of using StartReadThread() inside gdb-remote comms to take care of reading the data for concurrent process plugin instances)

... but we don't have such a case, so doing this might be fine. I sort of understand the direction you're going with this, but I can't really visualize the final state. Could you give a short summary of the desired final architecture.

FWIW, my idea was to use the gdb-remote "async" thread for this centralized sort of reading(sending)/writing(receiving). The async thread serves a similar purpose to the communication read thread, but is completely unrelated to it. I thought that would be the easiest to implement, since we already kind of have a synchronization mechanism (the "continue" lock) between these thread and other threads. However, I don't have a complete picture of how would this work (I'd guess we have a single "async" thread for all of the processes, and it would somehow synchronize/arbitrate between them), so I am open to other ideas as well.

Well, my "baseline" idea was something like that: having a shared (e.g. via shared_ptr) m_gdb_comm, and running async thread as part of that. The thread would read packets from remote and split them into two groups: asynchronous events (i.e. stop reasons and related packets) and synchronous replies. This is reasonably easy for non-stop protocol since the former are always sent as % notification, while the latter as regular packets. For non-nonstop case, I suppose we would just trigger some explicit switch on resuming/leaving resume. Asynchronous events would be passed (e.g. via callback) to process instances that are currently resumed. Synchronous replies would be just passed to whichever process is currently waiting for the reply.

However, the problem with that design is that it requires a lot of different changes, and the gdb-remote plugin code is not exactly friendly to that. So I was trying to split it into smaller steps that would be testable and reviewable on their own, i.e. basically refactor, refactor, get multiprocess working, cleanup, cleanup. What these changes were aiming at was basically moving all the reads and writes from gdb-remote into a dedicated thread reusing the code already in LLDB, so I could initially add support for the new "async" thread and handling the resume-related packets there without having to immediately replace the existing code for synchronous packets that reads and writes to m_gdb_comm directly from a lot of places. However, now that I think of it, I didn't really think it through and using read thread along with some trivial locking and queues won't be sufficient to be able to cleanly supports reading these two kinds of packets from different threads.

At this point I'm basically lost in the woods and I'm not really sure how to proceed, or how much of this code is actually worth further consideration.

Ok, so I guess I'm back the original plan of using async thread to do the packet exchanges. Right now, the rough plan is to:

  1. Try to clean up / streamline the comms API a bit.
  2. Introduce a wrapper between the client and the actual comms to clearly split the public packet exchange API from the actual comms. Initially, the wrapper will just call into the comm methods.
  3. Add the "new" async thread to the wrapper. The actual comms will be performed by the thread, and the wrapper will communicate with it.
  4. Add support for sharing the wrapper between multiple process instances, and using the comm thread from multiple processes simultaneously.

Exact details TBD. It's possible that initially I'll leave the per-process "old" async threads as well in order to reduce the diffs.

labath added a comment.Sep 5 2022, 6:02 AM

Ok, so I guess I'm back the original plan of using async thread to do the packet exchanges. Right now, the rough plan is to:

  1. Try to clean up / streamline the comms API a bit.
  2. Introduce a wrapper between the client and the actual comms to clearly split the public packet exchange API from the actual comms. Initially, the wrapper will just call into the comm methods.
  3. Add the "new" async thread to the wrapper. The actual comms will be performed by the thread, and the wrapper will communicate with it.
  4. Add support for sharing the wrapper between multiple process instances, and using the comm thread from multiple processes simultaneously.

Exact details TBD. It's possible that initially I'll leave the per-process "old" async threads as well in order to reduce the diffs.

Seems reasonable. Just be aware that the packet RTT is very important for some, so we will either make sure that the extra thread is not used in the base case (no processes running), or somehow ensure that the extra thread does not introduce delays.

mgorny added a comment.Sep 5 2022, 6:56 AM

Seems reasonable. Just be aware that the packet RTT is very important for some, so we will either make sure that the extra thread is not used in the base case (no processes running), or somehow ensure that the extra thread does not introduce delays.

I'll bear that in mind. Worst case, the wrapper class can take care of conditionally enabling the extra thread and falling back to invoking comms directly.

mgorny updated this revision to Diff 458017.Sep 5 2022, 8:41 AM

Rebase on top of D131160.