> 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
