[Lldb-commits] [PATCH] Implements a HostThread class.

[Todd Fiala]

Great thanks!


On Fri, Sep 5, 2014 at 4:42 PM, Zachary Turner <zturner at google.com> wrote:

> Yes, sorry for the confusion.  There's a totally new review up.  I
> commented in this thread yesterday saying that I uploaded a new review, but
> since it was completely different it was in a new issue entirely.  Then
> comments about that review started coming in on this thread.  Anyway, look
> here: http://reviews.llvm.org/D5198
>
>
> On Fri, Sep 5, 2014 at 4:35 PM, Todd Fiala <tfiala at google.com> wrote:
>
>> Hey Zachary - am I looking in the right spot?  The top of the thread is
>> talking about this:
>>
>> http://reviews.llvm.org/D5110
>>
>> You said this:
>>
>> > Latest version is up, should address all the issues pointed out in
>> yoru comments.
>>
>> Which leads me to believe I would see multiple versions of it.  I'm only
>> seeing the August patch on reviews.llvm.org.  What am I missing?  (I'm
>> guessing it's the wrong review?)
>>
>> -Todd
>>
>>
>> On Fri, Sep 5, 2014 at 3:31 PM, Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> Latest version is up, should address all the issues pointed out in yoru
>>> comments.  I didn't actually address the HostThread::GetName() issue in
>>> this patch, because it turns out we only ever attempt to get the thread
>>> name in one place, and it's of the current thread.  I still am fine
>>> changing this, but the CL is already pretty huge and it didn't seem too
>>> urgent for the purposes of this change.
>>>
>>>
>>> On Fri, Sep 5, 2014 at 9:17 AM, <jingham at apple.com> wrote:
>>>
>>>> Yes, the "alternate" method sounds fine to me as well.
>>>>
>>>> Jim
>>>>
>>>>
>>>> > On Sep 4, 2014, at 6:35 PM, Zachary Turner <zturner at google.com>
>>>> wrote:
>>>> >
>>>> > Come to think of it I actually like my "alternate" method more than
>>>> the way I've done it, because it means I don't need to duplicate handles in
>>>> the assignment / copy-constructor of HostThreadWindows, because the only
>>>> thing that will get copied is the shared_ptr.
>>>> >
>>>> >
>>>> > On Thu, Sep 4, 2014 at 6:18 PM, Zachary Turner <zturner at google.com>
>>>> wrote:
>>>> >
>>>> >
>>>> >
>>>> > On Thu, Sep 4, 2014 at 6:06 PM, <jingham at apple.com> wrote:
>>>> >
>>>> > > On Sep 4, 2014, at 5:57 PM, Zachary Turner <zturner at google.com>
>>>> wrote:
>>>> > >
>>>> > > Regarding point #1: I'm still not sold on this.  Exposing only the
>>>> HostThreadBase really complicates some things.  Some of the issues escape
>>>> my mind right now, but one in particular stands out.  There are a couple of
>>>> places where threads are copied.  I don't remember the exact file off the
>>>> top of my head, but something about making a copy of a thread in case the
>>>> thread is shutting down and nulls itself out.  With a pointer or reference
>>>> to a base class, this can't be done without a virtual Clone() method, which
>>>> is really kind of gross.
>>>> >
>>>> > I would like to see what is hard.  You are using a generic factory to
>>>> make the HostThreads, your ThreadRunner, and then you are calling generic
>>>> methods on them.  Maybe I'm too stuck on this, but it seems like we should
>>>> keep host specific stuff out of generic lldb functionality, and enforce
>>>> that with the compiler, not with buildbots on the lacking host failing
>>>> sometime later on...  That just seems ugly to me.
>>>> >
>>>> > It's not that it's hard, I guess it's just a difference of opinion on
>>>> what's the most ugly.  Consider the following block of code which uses raw
>>>> lldb:thread_t's.
>>>> >
>>>> > lldb::thread_t backup = m_thread;
>>>> > ...
>>>> > m_thread = backup;
>>>> >
>>>> > With my patch, that becomes the following:
>>>> >
>>>> > HostThread backup = m_thread;
>>>> > ...
>>>> > m_thread = backup;
>>>> >
>>>> > With the base class method, that becomes the following:
>>>> >
>>>> > HostThreadBaseSP backup = m_thread->Clone();
>>>> > ...
>>>> > m_thread = backup->Clone();
>>>> >
>>>> > These look similar, but behind the scenes it's ugly.  You now need a
>>>> pure virtual Clone() method on HostThreadBase (trivial to implement of
>>>> course, but it's just code pollution).  You need to store by pointer
>>>> instead of by value, which means you have to worry about null-checks as
>>>> well.
>>>> >
>>>> > There is the issue you mention which is that only a buildbot will
>>>> tell you if you use a platform-specific method, but my argument is just
>>>> that this is strictly better than before, because before *nobody* would
>>>> tell you when you used a platform-specific method.  It would just be a
>>>> no-op.
>>>> >
>>>> > That said, I have another idea in case you're still opposed to the
>>>> way I've done it.  Basically, just have HostThread.  Nothing derives from
>>>> it.  Its only member is a shared_ptr<HostNativeThread>.  HostNativeThread
>>>> does have subclasses just as the current HostThreadBase does.  The methods
>>>> of HostThread just forward their calls to HostNativeThread, but also
>>>> HostThread provides a method called GetNativeThread() which returns
>>>> HostNativeThread automatically cast to the most-derived type.
>>>> >
>>>> > This way, in generic code, as long as you don't write
>>>> thread.GetNativeThread(), you're guaranteed to only be using generic
>>>> methods.
>>>> >
>>>>
>>>>
>>>
>>> _______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>
>>>
>>
>>
>> --
>> Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
>>
>
>


-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140905/c3eda2cb/attachment.html>