[Lldb-commits] [PATCH] Implements a HostThread class.

[Zachary Turner]

Sorry, I meant a constructor that takes a tid_t seems useful.


On Fri, Aug 29, 2014 at 12:38 PM, Zachary Turner <zturner at google.com> wrote:

> BTW, one thing I was stuck on in my original patch.  It seems I can go
> from a thread_t to a tid_t on MacOSX via pthread_threadid_np.  How do I go
> the other direction?  HostThreadBase is going to store a thread_t.  But a
> method that returns a tid_t seems generally useful.
>
>
> On Fri, Aug 29, 2014 at 12:34 PM, <jingham at apple.com> wrote:
>
>>
>> > On Aug 29, 2014, at 12:33 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> >
>> > I'll proceed as discussed in the earlier thread where I listed the
>> interfaces, minus the Suspend / Resume.  I'll keep an eye on this thread
>> though in case anything else comes up.  Thanks for trudging through this
>> with me!
>>
>> Sounds great!  More thanks always go to the fellow actually doing the
>> work!
>>
>> Jim
>>
>> >
>> >
>> > On Fri, Aug 29, 2014 at 12:31 PM, <jingham at apple.com> wrote:
>> > Yes, that sounds right.
>> >
>> > Jim
>> >
>> > > On Aug 29, 2014, at 12:30 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> > >
>> > > Thinking about it some more, removing SuspendThread and ResumeThread
>> seems reasonable.  Functionality beyond what is required for dealing with
>> own-process threads can be in a different abstraction which is backed by a
>> HostThread through containment.
>> > >
>> > >
>> > > On Fri, Aug 29, 2014 at 12:12 PM, <jingham at apple.com> wrote:
>> > > Yes, I think there's enough reason to expect we'll use HostThread in
>> ways the C++11 standards committee didn't think of that keeping a wrapper
>> is useful.  Also, Hosts mostly have to have fairly good thread libraries
>> now-a-days, but they don't necessarily have to have good C++11 std::thread
>> implementations, so insulating ourselves here is probably not a bad idea.
>> OTOH, Greg has been making rumblings about backing the Host thread
>> functionality by std::thread's ever since we decided to require C++11.
>> > >
>> > > Among other teenie reasons for keeping the wrapper, std::thread's
>> don't have "set name".  That may seem trivial, but we get crash logs from
>> Xcode where Xcode is running many lldb Debugger sessions simultaneously
>> (the Swift Playground feature is one among many causes of this.)  These
>> CrashLogs were really hard to read till we got both Xcode and lldb to name
>> all the threads that belong to a given process with the PID of the
>> process.  I would not like to lose this feature...
>> > >
>> > > Jim
>> > >
>> > >
>> > > > On Aug 29, 2014, at 11:59 AM, Zachary Turner <zturner at google.com>
>> wrote:
>> > > >
>> > > > Actually, disregard my last message.  I agree with you, the things
>> you do when you manipulate threads in a debuggee are different.  So for
>> that you have some other abstraction, but that other abstraction can still
>> be *backed by* a HostThread
>> > > >
>> > > >
>> > > > On Fri, Aug 29, 2014 at 11:54 AM, Zachary Turner <
>> zturner at google.com> wrote:
>> > > > If that's the case, is there any reason to not just use std::thread
>> directly?
>> > > >
>> > > >
>> > > > On Fri, Aug 29, 2014 at 11:51 AM, <jingham at apple.com> wrote:
>> > > > So thread_t says:
>> > > >
>> > > > //  lldb::thread_t          The native thread type for spawned
>> threads on the system
>> > > >
>> > > > I am pretty sure that was only ever meant to be used for threads
>> inside of the running lldb.  It seems to me that it will make things very
>> confusing to try to use it for random threads in some other process.  The
>> way you manipulate your own threads, and the things you can do with them,
>> are very different from the way you manipulate threads in the debugee, and
>> what is desirable.  Unless there's some compelling reason that I don't see
>> right now, I'd like to keep these two concepts separate.
>> > > >
>> > > > Jim
>> > > >
>> > > >
>> > > > > On Aug 29, 2014, at 11:35 AM, Zachary Turner <zturner at google.com>
>> wrote:
>> > > > >
>> > > > > I mentioned this a few times earlier, but admittedly the thread
>> has grown pretty long.
>> > > > >
>> > > > > HostThread is a replacement for thread_t.  Anywhere you use a
>> thread_t, you can use a HostThread.  This applies obviously to threads
>> inside the debugger itself, but also to any other threads on the machine.
>> In the case of debugging process P locally, for example, it might be used
>> when viewing or manipulating the threads of P.  Even in remote debuggign
>> scenarios, the debugserver might use a HostThread to manipulate threads in
>> the inferior.  There's already a method in Host called FindProcessThreads,
>> for example.  Instead of returning a list of thread ids, it could return a
>> list of HostThreads.  The size of the class should not be much (if any)
>> larger than an lldb::thread_t, it just has extra methods for convenience.
>> > > > >
>> > > > >
>> > > > > On Fri, Aug 29, 2014 at 11:28 AM, <jingham at apple.com> wrote:
>> > > > > One thing I am unclear about, I had thought that HostThread in
>> particular would be used for threads IN lldb, but not for handling threads
>> in a program lldb is debugging when on that Host.  So for instance, Suspend
>> & Resume don't seem necessary or desirable when dealing with threads lldb
>> is using for implementing lldb, and Launch is possible but quite tricky and
>> not required for remote threads on the same Host.
>> > > > >
>> > > > > What is the actual story here?
>> > > > >
>> > > > > Jim
>> > > > >
>> > > > >
>> > > > > > On Aug 29, 2014, at 11:19 AM, Zachary Turner <
>> zturner at google.com> wrote:
>> > > > > >
>> > > > > > What do you think about this?  I implement the abstract base
>> class.  I'll give it the following methods:
>> > > > > >
>> > > > > > Pure virtual:
>> > > > > > * Launch
>> > > > > > * Join
>> > > > > > * Cancel
>> > > > > > * GetState (enumerated value : invalid, running, exited,
>> suspended, cancelled)
>> > > > > > * Suspend
>> > > > > > * Resume
>> > > > > > * GetThreadResult
>> > > > > >
>> > > > > > Virtual with default implementations:
>> > > > > > * GetName
>> > > > > > * SetName
>> > > > > >
>> > > > > > Cancel will return an lldb::Error if cancellation is not
>> supported (for example because we're on windows and we don't own the thread
>> routine).  If you think of any other methods that should go here, let me
>> know.
>> > > > > >
>> > > > > > Initial patch will go in like this, with no code yet updated to
>> use the new HostThread.  In a subsequent patch, I will change thread_t's to
>> HostThreads, updating code as necessary.  We can evaluate the decision
>> about the typedef vs. the base class at this point, when we see what (if
>> any) impact this will have on whether specific methods will be accessed
>> from generic code.  At this point, if there's still disagreement, I can
>> even make two separate patches - one that does it each way, so that we can
>> compare actual code that uses both methods.
>> > > > > >
>> > > > > >
>> > > > > > On Fri, Aug 29, 2014 at 11:01 AM, <jingham at apple.com> wrote:
>> > > > > >
>> > > > > > > On Aug 29, 2014, at 10:09 AM, Zachary Turner <
>> zturner at google.com> wrote:
>> > > > > > >
>> > > > > > > On Fri, Aug 29, 2014 at 9:54 AM, <jingham at apple.com> wrote:
>> > > > > > >
>> > > > > > > > SystemLog
>> > > > > > > > ThreadDetach
>> > > > > > > > ThreadCancel
>> > > > > > >
>> > > > > > > There must be some way on Windows to tell a thread to exit at
>> the next cancellation point?  Can you really not cancel a worker thread's
>> operation?
>> > > > > > >
>> > > > > > > Not an arbitrary thread, no.  You can forcefully terminate it
>> (with caveats, and it may not work and/or have unexpected behavior, so it
>> is strongly discouraged), but generally if you want to gracefully cancel a
>> thread, the thread has to have specific code in its run loop to make that
>> possible.  So it might work for threads that we create inside the lldb
>> process, since we control the thread routine, but it wouldn't be meaningful
>> for threads in other process.  The reason for this discrepancy is that
>> there is no concept of signals on Windows, which I assume is how thread
>> cancellation is implemented behind the scenes on posix platforms.
>> > > > > >
>> > > > > > I don't think thread cancellation requires signals for same
>> process thread cancellation for pthreads, but it does use kernel support.
>> The system defines "cancellation points" - e.g. select, read, etc. where a
>> thread is likely to sit waiting, and pthread_cancel just marks the target
>> thread so that if it is in a cancellation routine (they're pretty much all
>> kernel traps) it gets killed off, otherwise the request is postponed till
>> the thread enters a cancellation point and then it gets killed.   I don't
>> think you would need signals for this.
>> > > > > >
>> > > > > > But it is something we need to do for all our ports, since we
>> rely on this for shutting down the Process worker threads cleanly.  Since
>> we only really care about canceling threads we create, I think it would be
>> okay for ThreadCancel to be a no-op for other threads.
>> > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > > GetAuxvData
>> > > > > > > >
>> > > > > > > > Before my original HostInfo refactor, it also had these
>> methods:
>> > > > > > > >
>> > > > > > > > GetUserID
>> > > > > > > > GetGroupID
>> > > > > > > > GetEffectiveUserID
>> > > > > > > > GetEffectiveGroupID
>> > > > > > > >
>> > > > > > > > Those methods were just as accessible to anyone writing
>> generic code.  They are *less* accessible now, because they are on
>> HostInfoPosix.  And this happened without needing to introduce any platform
>> specific pre-processor defines into generic LLDB.  There was maybe one
>> exception, which Jason Molenda pointed out earlier, which was the
>> GetOSBuildString.  And that only happened because GetOSBuildString is not a
>> generic concept!   The design worked exactly as intended, exposing a place
>> where code that was intended to be generic actually wasn't as generic as it
>> thought it was.  Everywhere else, the GetUserID, GetGroupID, etc methods
>> were only being called from specific code (which is how it should work),
>> but after my change this is now enforced by the compiler.
>> > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > > > But again, if I'm writing code it seems like a real pain to
>> have to answer the question "is this an interface I can use in generic
>> code" be:
>> > > > > > >
>> > > > > > > For now you can if it exists in all the HostThreadXXX.h
>> files, but of course if somebody introduces another platform and decides
>> that they don't want to implement this function then you can't use it
>> anymore and have to put
>> > > > > > >
>> > > > > > > #if defined
>> > > > > > >
>> > > > > > > guards around the usage.  Instead, we should look at the host
>> interfaces and say there are three classes of things:
>> > > > > > >
>> > > > > > > 1) Things you must implement in order to port lldb to a host
>> > > > > > >     These should be pure virtual methods in HostFeature.h
>> > > > > > > 2) Things you can optionally implement, but there's a
>> reasonable "couldn't do that" fallback
>> > > > > > >     These should be virtual methods in HostFeature.h
>> > > > > > > 3) Things that are purely host specific.
>> > > > > > >     These should be methods in HostFeatureMyHost.h, and can
>> only be used in HostOtherFeatureMyHost.h, but never in generic code.
>> > > > > > >
>> > > > > > > This would make the job of folks working in generic code
>> clear, and also make it obvious to the next porter (OpenVMS, anyone?) what
>> they have to do.
>> > > > > > >
>> > > > > > > I'm ok with doing all of this.  I'm all for having the
>> compiler catch things for you, and if one of the things it can catch for
>> you is "you need to implement this method" then that's great.  That said,
>> I'm still rather fond of the idea of typedefing HostFeature to
>> HostFeatureMyHost and then having everyone use HostFeature.  No matter if
>> you do it or don't, it's still equally easy to write specific code from
>> generic code.  You could just do this:
>> > > > > > >
>> > > > > > > #if defined(__APPLE__)
>> > > > > > > ((HostFeatureMacOSX&)feature).AppleSpecificMethod();
>> > > > > > > #endif
>> > > > > > >
>> > > > > > > as opposed to this
>> > > > > > >
>> > > > > > > #if defined(__APPLE__)
>> > > > > > > feature.AppleSpecificMethod();
>> > > > > > > #endif
>> > > > > > >
>> > > > > >
>> > > > > > My feeling about this is since you'd actually have to do:
>> > > > > >
>> > > > > > #if defined(__APPLE__)
>> > > > > > #include "lldb/Host/windows/HostFeatureMacOSX.h"
>> > > > > > (((HostFeatureMacOSX&)feature).AppleSpecificMethod();
>> > > > > > #endif
>> > > > > >
>> > > > > > I hope at that point you'd know you've gone off the reservation.
>> > > > > >
>> > > > > > Jim
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > > In both cases though, the person has felt strongly enough
>> about it to put it in a pre-processor guard, so the decision has already
>> been made.  And the second method has the benefit that when you're writing
>> specific code (which I anticipate to write alot of for Windows), you just
>> always have the type you need.
>> > > > > >
>> > > > > >
>> > > > >
>> > > > >
>> > > >
>> > > >
>> > > >
>> > >
>> > >
>> >
>> >
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140829/643e3987/attachment.html>