> On Aug 29, 2014, at 1:14 PM, Zachary Turner <[email protected]> wrote: > > 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. > >
Sounds reasonable. Jim > On Fri, Aug 29, 2014 at 12:58 PM, <[email protected]> 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 <[email protected]> wrote: > > > > Sorry, I meant a constructor that takes a tid_t seems useful. > > > > > > On Fri, Aug 29, 2014 at 12:38 PM, Zachary Turner <[email protected]> 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, <[email protected]> wrote: > > > > > On Aug 29, 2014, at 12:33 PM, Zachary Turner <[email protected]> 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, <[email protected]> wrote: > > > Yes, that sounds right. > > > > > > Jim > > > > > > > On Aug 29, 2014, at 12:30 PM, Zachary Turner <[email protected]> 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, <[email protected]> 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 <[email protected]> > > > > > 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 <[email protected]> > > > > > 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, <[email protected]> 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 <[email protected]> > > > > > > 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, <[email protected]> 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 <[email protected]> > > > > > > > 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, <[email protected]> wrote: > > > > > > > > > > > > > > > On Aug 29, 2014, at 10:09 AM, Zachary Turner > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > On Fri, Aug 29, 2014 at 9:54 AM, <[email protected]> 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
