Fix a bug where lldb does not respect the packet size.

[abidh]

Summary

LLDB was using packet size advertised by the target as the max memory size to write in one go. It is wrong because packets have other overhead apart from memory payload. Also memory transferred through 'm' and 'M' packets needs 2 bytes in packet to transfer 1 of memory.

[clayborg]

We need to check stub_max_size to make sure we don't get an integer underflow.

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

3999

You need to check "sub_max_size" here. What if it says 64? We will then use a very large unsigned number UINT64_MAX - 6.

[abidh]

Added the check to avoid integer underflow.

[abidh]

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

3999

Thanks for review. I had a check in my development branch then removed it as the scenario seemed unlikely and there was not a good way to return error from this function. I was inclined to put this check in DoMemoryWrite/read and check the exact size every time but it looks overkill. The check added in this revision probably should be enough.

[clayborg]

A few cleanups on the logging. See inlined comments.

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

4004

We should use a GDB remote log channel instead of the LLDB channel. Since this is communication based I suggest:

Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_COMM| GDBR_LOG_MEMORY));

4006–4007

Missing space between "small." and "LLDB". We should also warn once, not every time.

[abidh]

Updated log calls as advised.

[abidh]

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

4006–4007

This function has a check at the top so the packet size is checked only once.

[abidh]

Greg, any further comment on this patch.

[clayborg]

See easy inline fix and this will be good to go

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

4004

LogIfAny, not LogIfAll. Then either bit will work to show this.

[abidh]

Use GetLogIfAnyCategoryIsSet as advised in comments.

[clayborg]

Looks good.

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

 }
 
 //------------------------------------------------------------------
 // Process Memory
 //------------------------------------------------------------------
 size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
                                       Error &error) {
   GetMaxMemorySize();
-  if (size > m_max_memory_size) {
+  bool binary_memory_read = m_gdb_comm.GetxPacketSupported();
+  // M and m packets take 2 bytes for 1 byte of memory
+  size_t max_memory_size =
+      binary_memory_read ? m_max_memory_size : m_max_memory_size / 2;
+  if (size > max_memory_size) {
     // Keep memory read sizes down to a sane limit. This function will be
     // called multiple times in order to complete the task by
     // lldb_private::Process so it is ok to do this.
-    size = m_max_memory_size;
+    size = max_memory_size;
   }
 
   char packet[64];
   int packet_len;
-  bool binary_memory_read = m_gdb_comm.GetxPacketSupported();
   packet_len = ::snprintf(packet, sizeof(packet), "%c%" PRIx64 ",%" PRIx64,
                           binary_memory_read ? 'x' : 'm', (uint64_t)addr,
                           (uint64_t)size);
   assert(packet_len + 1 < (int)sizeof(packet));
   UNUSED_IF_ASSERT_DISABLED(packet_len);
   StringExtractorGDBRemote response;
   if (m_gdb_comm.SendPacketAndWaitForResponse(packet, response, true) ==
       GDBRemoteCommunication::PacketResult::Success) {
if (m_gdb_comm.SendPacketAndWaitForResponse(packet, response, true) ==
     error.SetErrorStringWithFormat("failed to send packet: '%s'", packet);
   }
   return 0;
 }
 
 size_t ProcessGDBRemote::DoWriteMemory(addr_t addr, const void *buf,
                                        size_t size, Error &error) {
   GetMaxMemorySize();
-  if (size > m_max_memory_size) {
+  // M and m packets take 2 bytes for 1 byte of memory
+  size_t max_memory_size = m_max_memory_size / 2;
+  if (size > max_memory_size) {
     // Keep memory read sizes down to a sane limit. This function will be
     // called multiple times in order to complete the task by
     // lldb_private::Process so it is ok to do this.
-    size = m_max_memory_size;
+    size = max_memory_size;
   }
 
   StreamString packet;
   packet.Printf("M%" PRIx64 ",%" PRIx64 ":", addr, (uint64_t)size);
   packet.PutBytesAsRawHex8(buf, size, endian::InlHostByteOrder(),
                            endian::InlHostByteOrder());
   StringExtractorGDBRemote response;
   if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString(), response,
if (stub_max_size != UINT64_MAX && stub_max_size != 0) {
       m_remote_stub_max_memory_size = stub_max_size;
 
       // Even if the stub says it can support ginormous packets,
       // don't exceed our reasonable largeish default packet size.
       if (stub_max_size > reasonable_largeish_default) {
         stub_max_size = reasonable_largeish_default;
       }
 
+      // Memory packet have other overheads too like Maddr,size:#NN
+      // Instead of calculating the bytes taken by size and addr every
+      // time, we take a maximum guess here.
+      if (stub_max_size > 70)
+        stub_max_size -= 32 + 32 + 6;
+      else {
+        // In unlikely scenario that max packet size is less then 70, we will
+        // hope that data being written is small enough to fit.
+        Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(
+            GDBR_LOG_COMM | GDBR_LOG_MEMORY));
+        if (log)
+          log->Warning("Packet size is too small. "
+                       "LLDB may face problems while writing memory");
+      }
+
       m_max_memory_size = stub_max_size;
     } else {
       m_max_memory_size = conservative_default;
     }
   }
 }
 
 void ProcessGDBRemote::SetUserSpecifiedMaxMemoryTransferSize(