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, <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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, <[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
