<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 17, 2014 at 3:11 PM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> On Sep 17, 2014, at 1:20 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
><br>
> +Jim, since he looked at my previous HostThread patch and some others, and may have additional context.<br>
><br>
> Just to be explicit, are you saying that I should not refactor any of this code, or that it's fine as long as the Apple stuff remains functionally equivalent?<br>
<br>
</span>I was out of of the office during the last patch for the threads. I know Jim did work with you on that one. A few things from the last patch that made me worry about a Process equivalent:<br>
<br>
class HostThread<br>
{<br>
public:<br>
HostThread();<br>
HostThread(lldb::thread_t thread);<br>
<br>
Error Join(lldb::thread_result_t *result);<br>
Error Cancel();<br>
void Reset();<br>
lldb::thread_t Release();<br>
<br>
void SetState(ThreadState state);<br>
ThreadState GetState() const;<br>
HostNativeThreadBase &GetNativeThread();<br>
const HostNativeThreadBase &GetNativeThread() const;<br>
lldb::thread_result_t GetResult() const;<br>
<br>
bool EqualsThread(lldb::thread_t thread) const;<br>
<br>
private:<br>
std::shared_ptr<HostNativeThreadBase> m_native_thread;<br>
};<br>
<br>
Why is there a separate HostNativeThreadBase class? Why not just make have each host implement the calls required in HostThread in their own respective HostThread.cpp class? Now we just indirect all calls through another class when there really isn't a valid reason to do so IMHO.<br></blockquote><div><br></div><div>Sorry, long story incoming.</div><div><br></div><div>Something like that was actually my first choice, and how I originally implemented it. There was HostThreadWindows, HostThreadPosix, HostThreadMac, etc. And then HostThread was typedefed to the appropriate implementation. We actually debated at length about whether it was the best approach, because people didn't like the typedef. Two alternative approaches were:</div><div><br></div><div>1) Use inheritance, make HostThreadBase provide only the generic operations, derived implementations can provide additional methods if they like</div><div>2) Make a single HostThread.h file with the interface, make multiple HostThread.cpp files each which implement the methods in HostThread.h differently.</div><div><br></div><div>I argued against 1 because I think it leads to some ugly code (for example, copy becomes clone, requires storing by pointer, requires null checking, requires factory-based creation, etc). We never really discussed 2, but I think the same arguments that people had against my original option with the typedef would have applied to #2 as well.</div><div> </div><div>But it probably helps to take a step back. From a high level perspective and ignoring any implementation details, what I'm trying to accomplish with this and other refactors is to create a strong separation between generic code and platform specific code. I want it to be <b>actually difficult</b> to call into platform-specific code from generic code. With the current design of Host, it's extremely easy. If you need a method, you add it to Host, and do something like this:</div><div><br></div><div>#if defined(MY_PLATFORM)</div><div>void Host::MyMethod()</div><div>{</div><div> // Implementation</div><div>}</div><div>#else</div><div>void Host::MyMethod()</div><div>{</div><div> // Nothing</div><div>}</div><div>#endif</div><div><br></div><div>This creates a problem in that a platform-specific operation is exposed through a supposedly generic interface. </div><div><br></div><div>The main argument against my original implementation with the typedef was that while it solves the problem somewhat (If you have HostThreadMacOSX::FooBar() but don't have HostThreadWindows::FooBar(), then writing HostThread::FooBar() on MacOSX will compile locally but break the buildbots, instead of silently passing and potentially introducing a bug in other platforms where the method was stubbed out), it could be made better by just never, for any platform, exposing a method that is not generic enough to actually work on every platform. This was the basis for Jim arguing for #1, and have everything use the base interface. This way it becomes very difficult to call a platform specific method. You have to include a platform specific header and cast to a platform specific type. </div><div><br></div><div>We arrived at the existing solution as sort of a compromise. The syntax is still nice, it's easily copyable just like an lldb::thread_t, and it doesn't expose any method unless that method can be implemented on every platform.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Why is there a SetState() and GetState() here? We set this to running when the thread handle is valid, even though it has no bearing on if the thread is actually running or suspended? Can we remove this?<br></blockquote><div>I can probably remove it. I provided it because it is possible to construct a HostThread with an arbitrary lldb::thread_t, and while in most cases it's safe to assume that the initial state is running, you could do some platform specific stuff with the handle before constructing it, and the class would be confused about the state.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Everything else is ok.<br>
<br>
The fallout from this was now any code that used to just check if a thread was valid, are now doing:<br>
<br>
bool<br>
Communication::JoinReadThread (Error *error_ptr)<br>
{<br>
if (m_read_thread.GetState() != eThreadStateRunning)<br>
return true;<br>
<br>
Error error = m_read_thread.Join(nullptr);<br>
m_read_thread.Reset();<br>
return error.Success();<br>
}<br>
<br>
<br>
Since this is now a class, we should probably just do:<br>
<br>
<br>
bool<br>
Communication::JoinReadThread (Error *error_ptr)<br>
{<br>
Error error = m_read_thread.Join(nullptr);<br>
m_read_thread.Reset();<br>
return error.Success();<br>
}<br>
<br>
And have join return success if the thread handle wasn't valid. The other reason I don't like the state in the HostThread is that it seems to indicate that this thread is actually running. I could have already exited, but if you ask the HostThread its state it will tell you "running" when it really isn't. Tracking when a thread goes away can get tricky so it would be hard to keep this state up to date. Can we change it back to a "bool IsValid() const" that returns true if the thread handle is valid?</blockquote><div> </div><div>Thinking about this, I kind of agree with you. In fixing up the code to use HostThread, I actually didn't notice anywhere where we care about anything other than whether or not the thread is running. So the other states don't seem to be that useful in practice. But is IsValid() really what we want to check? We really want to check if the thread is running. We own the thread routine because it's the ThreadCreateTrampoline. Can't we detect this by just having the HostThread look at the value of a shared variable?<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> I am not sure if you were thinking of adding more to this class. But this class isn't really designed to represent a thread from another process, so it isn't very useful in that respect. I seem to remember Jim saying that you thought that this class might be able to be reused in for other threads in other processes, and then backed back to your design, so there might just be stuff left over?<br></blockquote><div> </div><div>I was convinced the other way on that point, and now think that this class should not be used to represent a thread running in another process. The requirements and interface are too different.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
This is the main reason for my concern with the next patch.</blockquote><div><br></div><div>After we reach an agreement about the HostThread patch, I can go into more detail about my plan for HostProcess and how it would be integrated.</div><div><br></div><div><br></div><div>Thanks!</div><div>Zach</div></div></div></div>