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
