[lldb-dev] [PATCH] new utility methods in GDBRemoteCommunicationClient

Steve Pucci spucci at google.com
Fri Jan 24 14:47:52 PST 2014


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
---
>         ErrorDisconnected,  // We were disconnected
>         ErrorNoSequenceLock // We couldn't get the sequence lock for a
multi-packet request





On Fri, Jan 24, 2014 at 10:34 AM, Steve Pucci <spucci at google.com> wrote:

> OK, thanks Greg.  I'll incorporate that and resend the patch.
>
> - Steve
>
>
> On Fri, Jan 24, 2014 at 10:22 AM, Greg Clayton <gclayton at apple.com> wrote:
>
>> 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
>>
>>
>> On Jan 24, 2014, at 8:13 AM, Steve Pucci <spucci at google.com> wrote:
>>
>> > 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
>> > -    // <prefix>:000,<size>
>> > -    // <prefix>:<size>,<size>
>> > -    // <prefix>:<size>*2,<size>
>> > -    // <prefix>:<size>*3,<size>
>> > +    // return all of the output via a series of request packets of the
>> form
>> > +    // <prefix>0,<size>
>> > +    // <prefix><size>,<size>
>> > +    // <prefix><size>*2,<size>
>> > +    // <prefix><size>*3,<size>
>> >      // ...
>> >      // 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 <prefix>;
>> > +    // 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%
>> >
>> >
>> >
>> >
>> > On Fri, Jan 24, 2014 at 7:41 AM, Steve Pucci <spucci at google.com> wrote:
>> > Thanks, Abid.  Answers to your questions below.
>> >
>> > On Fri, Jan 24, 2014 at 2:11 AM, Abid, Hafiz <Hafiz_Abid at mentor.com>
>> wrote:
>> > 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
>> >
>> > +    // <prefix>:000,<size>
>> >
>> > +    // <prefix>:<size>,<size>
>> >
>> > +    // <prefix>:<size>*2,<size>
>> >
>> > +    // <prefix>:<size>*3,<size>
>> >
>> > +    // ...
>> >
>> > +    // 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.
>> >
>> >
>> > OK, will do, thanks.
>> >
>> >
>> >  +            printf("max packet size %lu\n", (unsigned
>> long)m_max_packet_size);
>> >
>> > You forgot to remove this printf.
>> >
>> >
>> > Right, thanks.
>> >
>> >
>> >  +        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?
>> >
>> >
>> > Yes, that's the way I have it currently in my calling code.  It's been
>> tested in that environment and works but I'm not ready to check in the
>> calling code yet.  (I've also tested it in an environment without my
>> calling code to ensure this works as a standalone patch, though it's
>> unused).
>> >
>> > 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?
>> >
>> >
>> > Yes, my calling code parses using libxml2, and the plan is to have
>> libxml2 conditionally compiled and linked in (using the same mechanism that
>> clang uses for the same purpose).  I have that working also in the calling
>> code.
>> >
>> > I considered putting the xml parser in GDBRemoteCommunicationClient
>> also but it seemed a bit higher-level than that class operates at.  But I'm
>> open to whatever people think here.  One possibility would be to split the
>> creation of the XML DOM and the walking of the DOM tree, and put the
>> creation of the DOM in GDBRemoteCommunicationClient.
>> >
>> > We are also interested in the xml target description but it's not a
>> priority at the moment.
>> >
>> > Thanks again for the review.  I'll send out the modified patch before
>> committing.
>> >
>> >  - Steve
>> >
>> >
>> >
>> >
>> > Regards,
>> >
>> > Abid
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > From: lldb-dev-bounces at cs.uiuc.edu [mailto:lldb-dev-bounces at cs.uiuc.edu]
>> On Behalf Of Steve Pucci
>> > Sent: 23 January 2014 21:04
>> > To: lldb-dev at cs.uiuc.edu
>> > Subject: [lldb-dev] [PATCH] new utility methods in
>> GDBRemoteCommunicationClient
>> >
>> >
>> >
>> > I'm working on better support for debugging against a remote stock
>> gdbserver, and these are some new support methods I'm using.  I'm sending
>> this separately for easier digestion.
>> >
>> >
>> >
>> > There are a couple of pieces:
>> >
>> >  * some lazy-evaluation members that store info listed in a qSupported
>> response
>> >
>> >  * new method SendPacketsAndConcatenateResponses which is used for
>> fetching fixed-size objects from the remote gdbserver by using multiple
>> packets if necessary (first use will be to fetch shared-library XML files).
>> >
>> >
>> >
>> > Thanks,
>> >
>> >    Steve
>> >
>> >
>> >
>> >
>> >
>> >
>> <patch-GDBRemoteCommClient-2.txt>_______________________________________________
>> > lldb-dev mailing list
>> > lldb-dev at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140124/e04070dc/attachment.html>
-------------- next part --------------
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp	(revision 200039)
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp	(working copy)
@@ -66,6 +66,9 @@
     m_prepare_for_reg_writing_reply (eLazyBoolCalculate),
     m_supports_p (eLazyBoolCalculate),
     m_supports_QSaveRegisterState (eLazyBoolCalculate),
+    m_supports_qXfer_libraries_read (eLazyBoolCalculate),
+    m_supports_qXfer_libraries_svr4_read (eLazyBoolCalculate),
+    m_supports_augmented_libraries_svr4_read (eLazyBoolCalculate),
     m_supports_qProcessInfoPID (true),
     m_supports_qfProcessInfo (true),
     m_supports_qUserName (true),
@@ -96,7 +99,8 @@
     m_os_build (),
     m_os_kernel (),
     m_hostname (),
-    m_default_packet_timeout (0)
+    m_default_packet_timeout (0),
+    m_max_packet_size (0)
 {
 }
 
@@ -149,6 +153,46 @@
 }
 
 bool
+GDBRemoteCommunicationClient::GetAugmentedLibrariesSVR4ReadSupported ()
+{
+    if (m_supports_augmented_libraries_svr4_read == eLazyBoolCalculate)
+    {
+        GetRemoteQSupported();
+    }
+    return (m_supports_augmented_libraries_svr4_read == eLazyBoolYes);
+}
+
+bool
+GDBRemoteCommunicationClient::GetQXferLibrariesSVR4ReadSupported ()
+{
+    if (m_supports_qXfer_libraries_svr4_read == eLazyBoolCalculate)
+    {
+        GetRemoteQSupported();
+    }
+    return (m_supports_qXfer_libraries_svr4_read == eLazyBoolYes);
+}
+
+bool
+GDBRemoteCommunicationClient::GetQXferLibrariesReadSupported ()
+{
+    if (m_supports_qXfer_libraries_read == eLazyBoolCalculate)
+    {
+        GetRemoteQSupported();
+    }
+    return (m_supports_qXfer_libraries_read == eLazyBoolYes);
+}
+
+uint64_t
+GDBRemoteCommunicationClient::GetRemoteMaxPacketSize()
+{
+    if (m_max_packet_size == 0)
+    {
+        GetRemoteQSupported();
+    }
+    return m_max_packet_size;
+}
+
+bool
 GDBRemoteCommunicationClient::QueryNoAckModeSupported ()
 {
     if (m_supports_not_sending_acks == eLazyBoolCalculate)
@@ -245,6 +289,9 @@
     m_supports_memory_region_info = eLazyBoolCalculate;
     m_prepare_for_reg_writing_reply = eLazyBoolCalculate;
     m_attach_or_wait_reply = eLazyBoolCalculate;
+    m_supports_qXfer_libraries_read = eLazyBoolCalculate;
+    m_supports_qXfer_libraries_svr4_read = eLazyBoolCalculate;
+    m_supports_augmented_libraries_svr4_read = eLazyBoolCalculate;
 
     m_supports_qProcessInfoPID = true;
     m_supports_qfProcessInfo = true;
@@ -260,9 +307,51 @@
     m_supports_QEnvironmentHexEncoded = true;
     m_host_arch.Clear();
     m_process_arch.Clear();
+
+    m_max_packet_size = 0;
 }
 
+void
+GDBRemoteCommunicationClient::GetRemoteQSupported ()
+{
+    // Clear out any capabilities we expect to see in the qSupported response
+    m_supports_qXfer_libraries_svr4_read = eLazyBoolNo;
+    m_supports_qXfer_libraries_read = eLazyBoolNo;
+    m_supports_augmented_libraries_svr4_read = eLazyBoolNo;
+    m_max_packet_size = UINT64_MAX;  // It's supposed to always be there, but if not, we assume no limit
 
+    StringExtractorGDBRemote response;
+    if (SendPacketAndWaitForResponse("qSupported",
+                                     response,
+                                     /*send_async=*/false) == PacketResult::Success)
+    {
+        const char *response_cstr = response.GetStringRef().c_str();
+        if (::strstr (response_cstr, "qXfer:libraries-svr4:read+"))
+            m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;
+        if (::strstr (response_cstr, "augmented-libraries-svr4-read"))
+        {
+            m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;  // implied
+            m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
+        }
+        if (::strstr (response_cstr, "qXfer:libraries:read+"))
+            m_supports_qXfer_libraries_read = eLazyBoolYes;
+
+        const char *packet_size_str = ::strstr (response_cstr, "PacketSize=");
+        if (packet_size_str)
+        {
+            StringExtractorGDBRemote packet_response(packet_size_str + strlen("PacketSize="));
+            m_max_packet_size = packet_response.GetHexMaxU64(/*little_endian=*/false, UINT64_MAX);
+            if (m_max_packet_size == 0)
+            {
+                m_max_packet_size = UINT64_MAX;  // Must have been a garbled response
+                Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS));
+                if (log)
+                    log->Printf ("Garbled PacketSize spec in qSupported response");
+            }
+        }
+    }
+}
+
 bool
 GDBRemoteCommunicationClient::GetThreadSuffixSupported ()
 {
@@ -364,6 +453,62 @@
 }
 
 GDBRemoteCommunicationClient::PacketResult
+GDBRemoteCommunicationClient::SendPacketsAndConcatenateResponses
+(
+    const char *payload_prefix,
+    std::string &response_string
+)
+{
+    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;
+    }
+
+    response_string = "";
+    std::string payload_prefix_str(payload_prefix);
+    unsigned int response_size = 0x1000;
+    if (response_size > GetRemoteMaxPacketSize()) {  // May send qSupported packet
+        response_size = GetRemoteMaxPacketSize();
+    }
+
+    for (unsigned int offset = 0; true; offset += response_size)
+    {
+        StringExtractorGDBRemote this_response;
+        // Construct payload
+        char sizeDescriptor[128];
+        snprintf(sizeDescriptor, sizeof(sizeDescriptor), "%x,%x", offset, response_size);
+        PacketResult result = SendPacketAndWaitForResponse((payload_prefix_str + sizeDescriptor).c_str(),
+                                                           this_response,
+                                                           /*send_async=*/false);
+        if (result != PacketResult::Success)
+            return result;
+
+        const std::string &this_string = this_response.GetStringRef();
+
+        // Check for m or l as first character; l seems to mean this is the last chunk
+        char first_char = *this_string.c_str();
+        if (first_char != 'm' && first_char != 'l')
+        {
+            return PacketResult::ErrorReplyInvalid;
+        }
+        // Skip past m or l
+        const char *s = this_string.c_str() + 1;
+
+        // Concatenate the result so far
+        response_string += s;
+        if (first_char == 'l')
+            // We're done
+            return PacketResult::Success;
+    }
+}
+
+GDBRemoteCommunicationClient::PacketResult
 GDBRemoteCommunicationClient::SendPacketAndWaitForResponse
 (
     const char *payload,
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h	(revision 200039)
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h	(working copy)
@@ -58,6 +58,27 @@
                                   StringExtractorGDBRemote &response,
                                   bool send_async);
 
+    // For packets which specify a range of output to be returned,
+    // return all of the output via a series of request packets of the form
+    // <prefix>0,<size>
+    // <prefix><size>,<size>
+    // <prefix><size>*2,<size>
+    // <prefix><size>*3,<size>
+    // ...
+    // until a "$l..." packet is received, indicating the end.
+    // (size is in hex; this format is used by a standard gdbserver to
+    // return the given portion of the output specified by <prefix>;
+    // 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);
+
     lldb::StateType
     SendContinuePacketAndWaitForResponse (ProcessGDBRemote *process,
                                           const char *packet_payload,
@@ -248,6 +269,9 @@
     const lldb_private::ArchSpec &
     GetProcessArchitecture ();
 
+    void
+    GetRemoteQSupported();
+
     bool
     GetVContSupported (char flavor);
 
@@ -359,6 +383,18 @@
     bool
     SetCurrentThreadForRun (uint64_t tid);
 
+    bool
+    GetQXferLibrariesReadSupported ();
+
+    bool
+    GetQXferLibrariesSVR4ReadSupported ();
+
+    uint64_t
+    GetRemoteMaxPacketSize();
+
+    bool
+    GetAugmentedLibrariesSVR4ReadSupported ();
+
     lldb_private::LazyBool
     SupportsAllocDeallocMemory () // const
     {
@@ -489,7 +525,10 @@
     lldb_private::LazyBool m_prepare_for_reg_writing_reply;
     lldb_private::LazyBool m_supports_p;
     lldb_private::LazyBool m_supports_QSaveRegisterState;
-    
+    lldb_private::LazyBool m_supports_qXfer_libraries_read;
+    lldb_private::LazyBool m_supports_qXfer_libraries_svr4_read;
+    lldb_private::LazyBool m_supports_augmented_libraries_svr4_read;
+
     bool
         m_supports_qProcessInfoPID:1,
         m_supports_qfProcessInfo:1,
@@ -532,6 +571,7 @@
     std::string m_os_kernel;
     std::string m_hostname;
     uint32_t m_default_packet_timeout;
+    uint64_t m_max_packet_size;  // as returned by qSupported
     
     bool
     DecodeProcessInfoResponse (StringExtractorGDBRemote &response, 
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h	(revision 200039)
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h	(working copy)
@@ -45,7 +45,8 @@
         ErrorReplyTimeout,  // Timed out waiting for reply
         ErrorReplyInvalid,  // Got a reply but it wasn't valid for the packet that was sent
         ErrorReplyAck,      // Sending reply ack failed
-        ErrorDisconnected   // We were disconnected
+        ErrorDisconnected,  // We were disconnected
+        ErrorNoSequenceLock // We couldn't get the sequence lock for a multi-packet request
     };
     //------------------------------------------------------------------
     // Constructors and Destructors


More information about the lldb-dev mailing list