[lldb-dev] gdb-remote command broken in TOT

[Todd Fiala]

This change is in at r208741.


On Tue, May 13, 2014 at 5:13 PM, Todd Fiala <tfiala at google.com> wrote:

> Thanks!
>
> And of course, Matthew, sorry for breaking you there.
>
>
> On Tue, May 13, 2014 at 5:12 PM, Greg Clayton <gclayton at apple.com> wrote:
>
>>
>> > On May 13, 2014, at 4:38 PM, Todd Fiala <tfiala at google.com> wrote:
>> >
>> > I should rephrase since we need context here:
>> >
>> > 1. TOT lldb <=> current debugserver/llgs
>> >
>> > Will use qProcessInfo and get the process id.
>> >
>> > 2. TOT lldb <=> older debugserver
>> >
>> > Will use qProcessINfo and get the process id.
>> >
>> > 3. TOT lldb <=> gdbserver
>> >
>> > Will fail to get the process id from qProcessInfo, will not ask for qC
>> because it's not Apple/iOS.  That will trigger lldb to think its not
>> attached properly.
>> >
>> > -- two fixes here:
>> > 3.1 - fall back to qC as we would in the Apple/iOS case, and realize we
>> wont' really have the real PID, which we don't really care about for the
>> remote setup case,
>> > 3.2 - rework and add a "do I have a process on the remote end"
>> predicate, and rework attach logic to just ask this new question which
>> doesn't promise a process id.
>>
>> I vote for 3.1 where we don't limit to iOS.
>>
>> > 4. old lldb <=> TOT debugserver
>> >
>> > Asks for qC, gets same behavior as it did before - gets a thread id,
>> treats it as a process id, connection works.
>>
>> Yes.
>> >
>> >
>> > So we really only have case 3 that is an issue, which is what Matthew
>> hit since he has a different  gdbremote.
>> >
>> > Since I believe that is the only case we're trying to fix here, I'd be
>> in favor of doing the simple fix of allowing $qC for a fake process id not
>> limited to Apple/iOS.
>>
>> Agreed.
>>
>> >
>> > That's my take.  I'm happy to check that change in if this seems
>> reasonable.
>> >
>> > -Todd
>> >
>> >
>> > On Tue, May 13, 2014 at 4:25 PM, Todd Fiala <tfiala at google.com> wrote:
>> > You realize that qC's response is not stable as a pseudo pid, right?
>>  So anything that asks for it, thinks they have it, and then asks again for
>> it can get a different result depending on what the gdb remote thinks is
>> the current thread.
>> >
>> > I'd offer we consider falling back to qC and not limiting it to iOS
>> over reverting this entirely, unless you strongly object.  *Or* we can
>> simply do exactly what you said - which is check if we're attached to
>> something - by grabbing the qC result and just not storing it as a process
>> id (i.e. essentially create a GDBRemoteCommunicationClient::HasProcess (),
>> which happens to be implemented in terms of checking qProcessInfo, then qC,
>> for anything).
>> >
>> > Just some thoughts.
>> >
>> > -Todd
>> >
>> >
>> > On Tue, May 13, 2014 at 4:05 PM, Greg Clayton <gclayton at apple.com>
>> wrote:
>> >
>> > > On May 13, 2014, at 7:56 AM, Todd Fiala <tfiala at google.com> wrote:
>> > >
>> > > Hey Matthew,
>> > >
>> > > > However is there any real need to restrict the fallback case to
>> just Apple/iOS?
>> > >
>> > > Keep in mind that qC in gdbserver does implement the protocol
>> correctly and returns the thread id, not the proc id. On Linux the first
>> thread happens to get a thread id that is identical to the proc id.  So
>> when launching an exe, the active thread id returned just happens to be the
>> proc id as well out of happy coincidence. This is not true on other OSes
>> and not guaranteed true when attaching to a Linux process if the active
>> thread is not the first thread.  Given that lldb was not caching the proc
>> id until my change last week, it means that lldb using qC against a
>> gdbserver (or gdb remote protocol implementing qC per spec) is definitely
>> getting the buggy behavior I was fixing (i.e. different proc ids throughout
>> the life of the process when running against a multi-threaded inferior, and
>> entirely invalid results for proc id when run on OSes where the thread ids
>> have no correlation to the proc id, as on MacOSX).
>> > >
>> > > Thus, reverting to using qC when qProcessInfo isn't implemented is
>> really just reverting to buggy and inconsistent behavior, although it may
>> allow lldb to run where it can't now (and could before) and the proc id
>> reported just happens to be right some of the time.
>> >
>> > I would revert this. The pid isn't really all that important, we just
>> need to know that we are talking to something valid or not. We assume zero
>> is invalid.
>> > >
>> > > I think the real fix here may be if $qProcessInfo isn't present, then
>> we probably need to settle for not assuming we can get the process id, and
>> just make sure lldb can handle running without it in that case.
>> > >
>> > > Greg, any thoughts on that?
>> >
>> > I would revert to getting the pid from qC. Lame, but it would make us
>> more backward compatible. Leave a note in the code saying we really prefer
>> stubs to implement the qProcessInfo packet.
>> >
>> > >
>> > > -Todd
>> > >
>> > > On Tuesday, May 13, 2014, Matthew Gardiner <mg11 at csr.com> wrote:e
>> thread if returned
>> > > Todd Fiala wrote:
>> > > This is a change I made.
>> > >
>> > > qC is supposed to return the thread id, not the process id.
>> > >
>> > > qProcessInfo is being used now to collect the process id.  We added a
>> fallback so that if the process info request fails, on Apple/iOS we fall
>> back to the older qC (which, as indicated before, used to return the
>> process id erroneously, see the gdb remote documentation).
>> > >
>> > > After doing a bit more analysis, I see that you added a fallback such
>> that if the process info request fails, we fall back to traditional
>> gdbserver qC. This is completely reasonable, and permits our custom stubs
>> to support qProcessInfo.
>> > >
>> > > However is there any real need to restrict the fallback case to just
>> Apple/iOS? that is:
>> > >
>> > >     const llvm::Triple &triple = GetProcessArchitecture().GetTriple();
>> > >     if ((triple.getVendor() == llvm::Triple::Apple) &&
>> > >         (triple.getOS() == llvm::Triple::IOS))
>> > >     {
>> > >
>> > > Surely, all stubs which fail to implement qProcessInfo should be
>> permitted to fallback to qC?
>> > >
>> > > I've just commented out the above "if" in my working copy, and now
>> the initial "gdb-remote host:port" works again - in so far as the current
>> inferior process number is reported rather than 0. (Admittedly other things
>> don't quite work as desired later on in the debug-session, e.g. "next". But
>> presumably an Apple/iOS stub would either fail similarly, or work as
>> expected due to some localised conditional treatment, (i.e. "if apple
>> then...")).
>> > >
>> > > So, to continue to interoperate with traditional gdbserver
>> implementations can we let all older stubs continue to fallback to qC?
>> > >
>> > > thanks
>> > > Matt
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > Member of the CSR plc group of companies. CSR plc registered in
>> England and Wales, registered number 4187346, registered office Churchill
>> House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United
>> Kingdom
>> > > More information can be found at www.csr.com. Keep up to date with
>> CSR on our technical blog, www.csr.com/blog, CSR people blog,
>> www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook,
>> www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at
>> www.twitter.com/CSR_plc.
>> > > New for 2014, you can now access the wide range of products powered
>> by aptX at www.aptx.com.
>> >
>> >
>> >
>> >
>> > --
>> > Todd Fiala |   Software Engineer |     tfiala at google.com |
>> 650-943-3180
>> >
>> >
>> >
>> >
>> > --
>> > Todd Fiala |   Software Engineer |     tfiala at google.com |
>> 650-943-3180
>> >
>>
>>
>
>
> --
> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>



-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140513/50264363/attachment.html>