This actually happens to be one of the challenges I find when we try to cut up patches a little too small or introduce concepts before use. It is easy to lose what the real functional purpose for a change is all about.
I tend to be a bit more in favor of seeing new ideas/refactorings/changes along with something that drove the change so that we don't lose track of the big picture. On Fri, 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 > > -- Todd Fiala | Software Engineer | [email protected] | 650-943-3180
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
