[PATCH] new utility methods in GDBRemoteCommunicationClient

Hi Steve,

I had a quick look at the patch. It mostly looks good. Some small comments below.

  • // For packets which specify a range of output to be returned,

  • // return all of the output via a series of packets of the form

  • // :000,

  • // :,

  • // :*2,

  • // :*3,

  • // …

  • // until a “$l…” packet is received, indicating the end.

  • // Concatenate the results together and return in response.

I think that perhaps you can make this comment a bit more clear. Just separate what the remote target will do and what this function will do.

  • printf(“max packet size %lu\n”, (unsigned long)m_max_packet_size);

You forgot to remove this printf.

  • snprintf(sizeDescriptor, sizeof(sizeDescriptor), “%x,%x”, offset, response_size);

  • PacketResult result = SendPacketAndWaitForResponse((payload_prefix_str + sizeDescriptor).c_str(),

  • this_response,

  • /send_async=/false);

When I see such packets in GDB remote log (e.g. $qXfer:features:read:64bit-core.xml:0,fff), there is a colon before offset. I don’t see it being added here. Is it supposed to be in payload_prefix_str?

I suppose after this is approved, you would be working on parsing the xml that came from the gdbserver. Would it be only for shared libraries or you are also interested in xml target description?

Regards,

Abid

Thanks, Abid. Answers to your questions below.

Hi,

A diff against the previous patch, incorporating Abid’s comments, follows inline. A modified full patch is included as an attachment.

Thanks,
Steve

diff --git a/usr/local/google/home/spucci/lldb/work/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 2026e50…a1e982b 100644
— a/usr/local/google/home/spucci/lldb/work/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -59,16 +59,22 @@ public:
bool send_async);

// For packets which specify a range of output to be returned,

  • // return all of the output via a series of packets of the form
  • // :000,
  • // :,
  • // :*2,
  • // :*3,
  • // return all of the output via a series of request packets of the form
  • // 0,
  • // ,
  • // *2,
  • // *3,
    // …
    // until a “$l…” packet is received, indicating the end.
  • // Concatenate the results together and return in response.
  • // If any packet fails, the response indicates that failure
  • // and the returned string value is undefined.
  • // (size is in hex; this format is used by a standard gdbserver to
  • // return the given portion of the output specified by ;
  • // for example, “qXfer:libraries-svr4:read::fff,1000” means
  • // "return a chunk of the xml description file for shared
  • // library load addresses, where the chunk starts at offset 0xfff
  • // and continues for 0x1000 bytes").
  • // Concatenate the resulting server response packets together and
  • // return in response_string. If any packet fails, the return value
  • // indicates that failure and the returned string value is undefined.
    PacketResult
    SendPacketsAndConcatenateResponses (const char *send_payload_prefix,
    std::string &response_string);
    diff --git a/usr/local/google/home/spucci/lldb/work/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

index 9f591c8…4a3292a 100644
— a/usr/local/google/home/spucci/lldb/work/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -341,7 +341,6 @@ GDBRemoteCommunicationClient::GetRemoteQSupported ()
{
StringExtractorGDBRemote packet_response(packet_size_str + strlen(“PacketSize=”));
m_max_packet_size = packet_response.GetHexMaxU64(/little_endian=/false, UINT64_MAX);

  • printf(“max packet size %lu\n”, (unsigned long)m_max_packet_size);
    if (m_max_packet_size == 0)
    {
    m_max_packet_size = UINT64_MAX; // Must have been a garbled response
    spucci-linux %emacs ~/lldb/svn% 1497%

patch-GDBRemoteCommClient-2.txt (11 KB)

Looks ok, but one fix is needed:

Since LLDB us multi-threaded, when you need to send more than one packet/response for a given command, you need to manually take the sequence mutex (which is recursive) to stop other threads from interfering with your packet sequence.

See the "GDBRemoteCommunicationClient::GetCurrentThreadIDs()" function for details.

You basically don't want this to happen:

thread 1 sends: qfThreadInfo
thread 2 sends: qfThreadInfo
thread 1 sends: qsThreadInfo
thread 2 sends: qsThreadInfo << possible error?

The sequence mutex guarantees all your packets go out in the correct order

thread 1: get sequence mutex
thread 1 sends: qfThreadInfo
thread 1 sends: qsThreadInfo
thread 1: release sequence mutex

We do have to watch out for when we are running because when running we do this:

thread 1: get sequence mutex
thread 1: send "c" packet to continue
....
thread 2: send 'qfThreadInfo' packet and get error back

Each packet can determine if it is ok to interrupt the process in order for it to send and this is a boolean that is included in:

GDBRemoteCommunicationClient::PacketResult
GDBRemoteCommunicationClient::SendPacketAndWaitForResponse
(
    const char *payload,
    size_t payload_length,
    StringExtractorGDBRemote &response,
    bool send_async
)

If "send_async" is true, then we will send a "\x03" to interrupt the process and stop it, then send the packet we wanted to send and get a response, and then continue the process.

The only packets that are currently allows to send as async are memory read/write and breakpoint/watchpoint set/clear.

So most commands try to get the sequence mutex and if that fails, they bail with an error as they should. This is something that is good to understand.

Greg

OK, thanks Greg. I’ll incorporate that and resend the patch.

  • Steve

Hi,

Here’s a new patch with the mutex check (full patch attached, diff against prior patch follows inline).

Thanks,
Steve

diff -r svn2/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp svn/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
461a462,472

Mutex::Locker locker;
if (!GetSequenceMutex(locker,
“ProcessGDBRemote::SendPacketsAndConcatenateResponses() failed due to not getting the sequence mutex”))
{
Log *log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_PROCESS | GDBR_LOG_PACKETS));
if (log)
log->Printf(“error: failed to get packet sequence mutex, not sending packets with prefix ‘%s’”,
payload_prefix);
return PacketResult::ErrorNoSequenceLock;
}

diff -r svn2/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h svn/llvm/tools/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
48c48,49
< ErrorDisconnected // We were disconnected

patch-GDBRemoteCommClient-3.txt (10.7 KB)

Looks good.