[lldb] [gdb-remote] Use Communication::WriteAll() over Write()

[mgorny]

Summary

Replace the uses of Communication::Write() with WriteAll() to avoid
partial writes. None of the call sites actually accounted for that
possibility and even if it is unlikely to actually happen, there doesn't
seem to be any real harm from using WriteAll() instead.

Ideally, we'd remove Write() from the public API. However, that would
change the API of SBCommunication. The alternative would be to alias it
to WriteAll().

Sponsored by: The FreeBSD Foundation

[labath]

Ideally, we'd remove Write() from the public API.

That might be a bit too extreme. Although I agree that the "All" version is what one wants in most cases, there are use cases (and plenty of historical precedent) for the other version.

I could imagine renaming things such that one is discouraged (or at least forced to think) from using automatically the Write function. Maybe call it WriteSome, WritePartial, ...

However we'd also need to think about consistency with all our other classes which have Write methods.

[mgorny]

In D132395#3742365, @labath wrote:

Ideally, we'd remove Write() from the public API.

That might be a bit too extreme. Although I agree that the "All" version is what one wants in most cases, there are use cases (and plenty of historical precedent) for the other version.

To be honest, I can't think of a real use case where aliasing Write() to WriteAll() would make a caller-visible difference. I mean, Write() could technically write less but isn't guaranteed to, right? So if Write() started using WriteAll() internally, would there be any real difference, except for the technical difference in how it works internally (and perhaps some performance loss if someone specifically didn't need the WriteAll() behavior)?

[labath]

To be honest, I can't think of a real use case where aliasing Write() to WriteAll() would make a caller-visible difference.

If you mean, "in the lldb code base", then you might be right, but generally speaking, the difference is in the handling of interrupts and blocking operations. After a select, a single write call is guaranteed not to block (and to write something), but that's is not the case if you're going to be calling it in a loop. I don't know if we have any code like that for writes, but we do rely on this pattern for reads (although we might also make the FD non-blocking -- I don't remember).

[mgorny]

Yeah, I meant inside LLDB, since that's what we're talking about. Basically, I'm wondering if we should perhaps rename WriteAll() to Write(), remove the old Write() method and just use the looped variant everywhere (implying also via SB*).

[labath]

In D132395#3745227, @mgorny wrote:

Yeah, I meant inside LLDB, since that's what we're talking about. Basically, I'm wondering if we should perhaps rename WriteAll() to Write(), remove the old Write() method and just use the looped variant everywhere (implying also via SB*).

Ok. In that case, I generally agree with you, but I am worried about consistency, not just with other Write methods, but also with all Read-like methods, and I'm not convinced that renaming just a single class will do more good than harm. But I would support a wider refactor of Read/Write like calls which makes it more obvious whether a particular call is able to complete with partial data.

[mgorny]

Yeah, you're right. This is not a problem worth addressing.

[labath]

I just ran into a similar problem, and I realized that (at least for reading) there are actually *three* variants of these calls that can be useful, depending on the case.

1. perform exactly one syscall, return what you get -- important when you don't want to block
2. read/write as much as you can, but also settle for less -- for example you may not know how much data there is, but you want to read all of it
3. read/write *exactly* the given data, error out if less -- when you know exactly how much you want to read, and anything less is useless

Implementing one on top of the other is easy, but conveying the desired semantics in a reasonably short name is not.

[labath]

Oh, and there's also 1b) read once, but retry in case of signals.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

for (;;) {
     if (read == 0) {
       break;
     }
     // write directly to stdin *this might block if stdin buffer is full*
     // TODO: enqueue this block in circular buffer and send window size to
     // remote host
     ConnectionStatus status;
     Status error;
-    m_stdio_communication.Write(tmp, read, status, &error);
+    m_stdio_communication.WriteAll(tmp, read, status, &error);
     if (error.Fail()) {
       return SendErrorResponse(0x15);
     }
   }
 
   return SendOKResponse();
 }
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Status ProcessGDBRemote::DoDeallocateMemory(lldb::addr_t addr) {
   return error;
 }
 
 // Process STDIO
 size_t ProcessGDBRemote::PutSTDIN(const char *src, size_t src_len,
                                   Status &error) {
   if (m_stdio_communication.IsConnected()) {
     ConnectionStatus status;
-    m_stdio_communication.Write(src, src_len, status, nullptr);
+    m_stdio_communication.WriteAll(src, src_len, status, nullptr);
   } else if (m_stdin_forward) {
     m_gdb_comm.SendStdinNotification(src, src_len);
   }
   return 0;
 }
 
 Status ProcessGDBRemote::EnableBreakpointSite(BreakpointSite *bp_site) {
   Status error;

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp

public:
   }
 
 protected:
   TestClient client;
   MockServer server;
 
   bool Write(llvm::StringRef packet) {
     ConnectionStatus status;
-    return server.Write(packet.data(), packet.size(), status, nullptr) ==
+    return server.WriteAll(packet.data(), packet.size(), status, nullptr) ==
            packet.size();
   }
 };
 } // end anonymous namespace
 
 // Test that we can decode packets correctly. In particular, verify that
 // checksum calculation works.
 TEST_F(GDBRemoteCommunicationTest, ReadPacket) {