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

[Zachary Turner]

Regarding point #1: I'm still not sold on this.  Exposing only the
HostThreadBase really complicates some things.  Some of the issues escape
my mind right now, but one in particular stands out.  There are a couple of
places where threads are copied.  I don't remember the exact file off the
top of my head, but something about making a copy of a thread in case the
thread is shutting down and nulls itself out.  With a pointer or reference
to a base class, this can't be done without a virtual Clone() method, which
is really kind of gross.

2) I forgot about that.  I actually deleted that code path from the MacOSX
side, so I'll add that back.

3) FWIW, the name ThisThread takes after the equivalent in STL.  In fact,
if it weren't for the fact that the main reason we use it is for getting
and setting the thread name, which STL doesn't support, we could just
delete it and use std::this_thread entirely.  See, for example, this page:
http://en.cppreference.com/w/cpp/thread/get_id.  At some poitn I planned to
wrap everything in host in an lldb_host namespace instead of the
lldb_private namespace, would that be sufficient to solvie the naming
issues?  On the other hand, I'm not really attached to the name ThisThread
either, I mostly just chose it because it's familiar to anyone who has used
c++11 threads before.  If you think I should change it, I'm fine with that.

4) This has the same caveat as mentioned in #3 - namely that I'd eventually
like to make an lldb_host namespace.  But again, not really opposed to
changing it either, except that if we assume the lldb_host namespace exists
today and choose the best name for the class based on the assumption that
it exists, then we would only have to deal with the code churn once, rather
than twice.

5) Sorry, this was actually a failing of clang-format.  It isn't smart
enough to know about the surrounding lines and it ignored the alignment.
 I'll fix this.

Finally, the deadlock I mentioned was introduced *by my* patch, and then
fixed in my follow-up.  So I don't think it needs a separate patch.  The
m_thread_list.Clear() was an *attempt* to fix the deadlock by guessing what
might be wrong, but didn't actually seem to have any effect.  That said, I
think the comment there is still correct, so the change should be left in.
 To elaborate, without the explicit clear, you rely on the order of
Process's implicit destruction of its members.  One is a a mutex, one is
the ThreadList.  If the Mutex gets destroyed first, the ThreadList will
then try to acquire the destroyed mutex, presumably leading to undefined
behavior.

As for the actual fix for the deadlock was that in the original patch, It
was in ProcessMonitor::StopMonitoringProcess().Before my patch, there was a
conditional "if (IS_VALID_LLDB_HOST_THREAD())", and in my patch I changed
it to "if (thread.GetState() != eThreadStateRunning)".  In other words, I
negated the condition.  BTW, for future reference, if you select two
specific diffs in Phabricator, such as the first two I uploaded, it will
show you the delta betwene those two.  In this case, it would have only
shown you 1 or 2 lines, one of which was me negating the bogus conditional.




On Thu, Sep 4, 2014 at 4:20 PM, <jingham at apple.com> wrote:

> Couple points:
>
> 1) It looks like even though you made a HostThreadBase, using the
> mechanism of your HostThread.h, it is still the Host specific class that
> you expose in generic code.  That seems to me to defeat the purpose of the
> base class, since it pollutes generic code with the specific
> HostThread<MyHost> type functions, which keeps the compiler from telling
> you whether you are using generic or host specific methods in generic
> code.  You introduced the base class, but didn't really use it to separate
> generic from specific behavior.
>
> I think it would be better to change HostThreadBase to HostThread, and
> that new generic HostThread.h is what should be included & used in generic
> code.  Then have HostThreadSpecific.h that contains the define of
> HostThread<HostName> to HostThreadSpecific.  When you need features of the
> specific host thread class that aren't in the generic base class, you
> include HostThreadSpecific.h which makes it clear that you are writing
> potentially non-portable code - which you really shouldn't have to do in
> generic lldb code.
>
> 2) I see there are some games you have to play with HostThread::SetName
> because Apple unfortunately implemented the pthread_setname_np not to take
> a thread ID but only to work on the current thread, which was kind of
> annoying of them...  You worked around that by implementing SetName for
> threads generically by using the HostThreadCreateInfo, and then having a
> static SetName function that wasn't in the HostThreadBase class.  That is
> sad but necessary, and not really a big deal, we're just saying you can't
> re-name threads mid-flight.
>
> OTOH, it seems like GetName can be implemented everywhere, so it should
> really should be in the HostThread class.  It would also be more convenient
> if it was an instance method on the HostThread.  Seems to me you will
> always have a HostThread when you go to use this, and it's weird to have to
> grab the ID from your HostThread object and pass it to a static function to
> get the name.
>
> 3) I'm not fond of the name ThisThread, since it doesn't tell me that this
> is a part of the host layer or what it does till I see it used.  Maybe
> "HostThreadSelf" would be better, that's pretty much the behavior it wraps.
>
> 4) I also don't think ThreadRunner is clear, both because it doesn't tell
> you that this is part of the host layer - given the other names in LLDB I'd
> expect it to be part of the ThreadPlans or something like that.  Also, this
> only launches threads, it doesn't actually have anything to do with their
> running once they are launched.  HostThreadLauncher might be better.
>
>
> 5) This one is pretty trivial, but there are a couple of places (e.g.
> Process.h and lldb-types.h) where you took a line like:
>
> typedef uintptr_t           thread_t;                   // Host thread type
>
> and changed it to:
>
> typedef void *thread_t;                                 // Host thread type
>
> The type change is fine, but since all the other lines around this one
> were laid out so that the define names all lined up, the white space change
> uglified the code...
>
> Finally, what was the part that fixed deadlocks in the Linux code, was it
> the m_thread_list.Clear() in Process.cpp?  When you check this in, can you
> break that bit out as a separate patch, not so much for review purposes as
> so that the SVN history will show what actually fixed this problem.
>
> Again, thanks for working on this,
>
> Jim
>
>
> > On Sep 4, 2014, at 2:26 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > 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/34febbbc/attachment.html>