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

Zachary Turner zturner at google.com
Tue Sep 2 11:39:28 PDT 2014 

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/20140902/9688bd4a/attachment.html>

More information about the lldb-commits mailing list