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

[Zachary Turner]

Just updating this thread since my new patch is up.  Unfortunately it's
huge.  I spent some time trying to break it down into smaller chunks, but
given the nature of the change it was quite difficult.  I think most of the
code under source/Plugins can be skipped over for the most part, if that's
any consolation.


On Tue, Sep 2, 2014 at 11:39 AM, Zachary Turner <zturner at google.com> wrote:

> Useful to know, and pretty much cements the idea that a class for managing
> threads in your own process should not be the same as a class for managing
> threads in another process.
>
>
> On Tue, Sep 2, 2014 at 11:38 AM, <jingham at apple.com> wrote:
>
>> Yes, Mac OS X actually has 3 different thread identifiers, which are used
>> for different purposes.  There's the mach thread port, the pthread id and
>> the universal thread ID.
>>
>> When working with your own threads - which is what the Host layer mostly
>> wants to do, the pthread id is the important one.
>>
>> For debugging, we don't use the pthread id for anything but display.  We
>> key off the unique id to get things like the thread name, queue name, etc.
>> And then the mach thread port is what you use to get the register state.
>>
>> Jim
>>
>> > On Sep 2, 2014, at 1:33 AM, Jean-Daniel Dupas <devlists at shadowlab.org>
>> wrote:
>> >
>> > One thing you should take in consideration is that on OS X, the type
>> used to manipulate remote thread (thread in an other process) is not the
>> same than the type used to manipulate in process thread.
>> >
>> > To manipulate remote thread, OS X uses mach_thread, and to manipulate
>> local thread, it uses POSIX thread. That why trying to handle different
>> concepts (lldb thread and other process thread) in the same class is not
>> necessarily what you want to do.
>> >
>> > Le 29 août 2014 à 22:14, Zachary Turner <zturner at google.com> a écrit :
>> >
>> >> HostThread is going to replace thread_t in the sense that you should
>> be able to use HostThread for everything.  But since thread_t is already
>> defined correctly on all platforms, I was just goign to have HostThreadBase
>> store the thread_t.  But I could just as easily have the various
>> implementations store the explicit type (HANDLE, pthread_t, etc) directly.
>> >>
>> >> I think it's useful to be able to have an empty HostThread object, for
>> the case where we want HostThread to represent a thread we didn't start.
>> Examples include the case where we have an InferiorThread class which
>> stores a HostThread as one of its members, or when you run a "thread list"
>> command and it builds a list of existing threads for some process.
>> >>
>> >>
>> >> On Fri, Aug 29, 2014 at 12:58 PM, <jingham at apple.com> wrote:
>> >> This was probably lacking because the only threads really managed by
>> the hosts' thread_t were ones we created, so you'd get your hands on a
>> thread_t by Host::ThreadCreate'ing it, then you could manipulate it with
>> Join, etc.  We didn't ever need to use thread_t for threads that already
>> existed in lldb.
>> >>
>> >> I don't quite understand:
>> >>
>> >> "HostThreadBase is going to store a thread_t"
>> >>
>> >> I thought to some extent HostThreadBase was going to replace
>> thread_t.  Then HostThreadPosix would store a ptid_t as a matter of its
>> implementation (and presumably a Handle on Windows?), but I'm not sure why
>> we would need to expose that.  If you wanted to get all the threads in
>> lldb, that seems to me something you'd ask the Host and it would return a
>> list of HostThreadBase's.
>> >>
>> >> BTW, in this context I wonder if we need Create, or if that should be
>> a constructor?  Would there be any use for an empty HostThread object that
>> you planned start up as a worker later, or would it be better to have a
>> NULL HostBase* (or empty unique pointer) and then construct into it?
>> >>
>> >> Jim
>> >>
>> >> > On Aug 29, 2014, at 12:39 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> >> >
>> >> > 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/20140904/9d997ac8/attachment.html>