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

[Zachary Turner]

Yep.  All that logic is copied over and should still work.  I actually even
made the name chopping a tad bit smarter.  AFAICT the only place the thread
name is retrieved on any platform is in the Logging code, and it's always
the name of the current thread.


On Fri, Sep 5, 2014 at 3:51 PM, Todd Fiala <tfiala at google.com> wrote:

> Yeah I'd vote to keep that separate.  We can always follow up with it.
>
> The place where the thread name is meaningful to my group is in the
> logging.  We've recently been adjusting thread naming sections of lldb to
> look at the max thread length when naming threads.  Right now the *BSD and
> Linux thread name max lengths are relatively meager (16 characters).  Any
> time the undotted portion of a thread name can be longer than that, we've
> been adding checks and using an alternative shorter naming that doesn't
> lose the key discriminator in the name (typically a pid or something).
>  (For dotted names, there is already a mechanism to shorten thread names by
> lopping off dotted sections --- we are only messing with the intra-dot
> portions).
>
> -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140905/8fef605f/attachment.html>