[lldb-dev] [PATCH] new utility methods in GDBRemoteCommunicationClient
Steve Pucci
spucci at google.com
Fri Jan 24 10:34:02 PST 2014
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/dbb386fe/attachment.html>
More information about the lldb-dev
mailing list