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

[jingham at apple.com]

> On Aug 28, 2014, at 5:20 PM, Zachary Turner <zturner at google.com> wrote:
> 
> I toiled over this question for a while, but ultimately decided against a base class with virtual methods because there will only ever be 1 type of HostThread per host platform.  For example, on MacOSX, the HostThread type is HostThreadMacOSX.  There's no need for polymorphism or the overhead that comes with virtual dispatch.  Furthermore it complicates things.  Suppose you have a HostThreadBase, and you pass a pointer to this into some mac specific class.  Now it has to cast, which is ugly.
> 
> It's true, there is a level of awkwardness that comes with collecting together the intersection of all the derived types.  However, it's something that we could do, for example, inside of HostThread.h and document the common interface there in a comment.  In the grand scheme of what I'm envisioning, you will need to use Host code from outside of Host (obviously), but this will be the exception, at well-defined entry points into the host layer, with a vast majority of host code being used from within itself.  This internal code would have access to the full interface since it's already under the assumption that it's compiling for the specific platform.
> 
> Let me know if it's not clear.

It is clear, but I don't think I agree with it.  

The overhead doesn't worry me, few if any of the host methods are going to be used in a tight loop where the cost of virtual methods will actually matter.

And I'm not sure I buy the "casting" awkwardness argument all that much, since for the most part the Host specific parts of the HostXXX implementations are going to be in the methods that implement each class - operating on "this" which is already of the full host specific type.  The number of times I will have to reach into the Host specific methods of an object not the one I'm implementing doesn't seem likely to be all that great.
 
Further, if the host interface is well designed, when I'm implementing a method of some Host specific class and I get handed a Host object which is not the one I'm currently implementing, the generic HostBase interface will probably do just fine.  Moreover, all the host specific methods that take sibling Host objects as parameters can take the full host specific version, and since that's more likely where you would need non-generic behaviors that would further reduce the need for casting.

OTOH, having to come up with a post-processing comment based way to say "This is the interface a new Host MUST provide in order to port LLDB to that host" seems like unnecessary work given that C++ provides a perfectly good way of doing that.  And having to use the buildbots to check whether I've violated the contract and used some HostThread function that exists on almost all platforms in generic code seems inelegant.

Also, I'd like it if generic lldb code didn't ever use Host specific code that wasn't part of the base host interface.  If you really need it you should do the work to make it generic.  Making the HostWhateverBase classes real in the header files that generic code includes would remove the temptation to just put in a little #if HOST_THREAD_TYPE ==.  You would have to actually include the host specific file, at which point you would know you were doing a bad thing...

Jim

 
> 
> 
> On Thu, Aug 28, 2014 at 4:40 PM, <jingham at apple.com> wrote:
> Maybe I need to wait till I see you use this, but I'm kind of surprised that the HostThread implementation has no base HostThread class here.  There is generic code in lldb that will need or want to use HostThreads.  For instance we have to create threads in lldb (e.g. Process has to create the thread that manages the internal state machine, and ProcessGDBRemote the reading thread) and we like to name the threads we create so that debugging lldb and reading Crash logs is easier, etc.
> 
> As you have it written, how do I know which HostThread methods it is safe to call in generic code?  And how do we make sure we have a common interface to these methods across all implementations.  Am I just supposed to read all the HostThreadXXX.h and use only the intersection of their public methods.  That seems awkward.
> 
> I expected to have a HostThread.h that declared as virtual methods all the thread operations you must provide to be a host onto which lldb can be ported, and the interface you must provide.  Maybe we could make a distinction between necessary (like Create) and useful (like Name) where the former has to work for lldb to function, and the latter can be no-op's.  Then the HostThreadXXX classes would derive from this, and add whatever native methods they offered, but those methods could only be called from HostXXX code.  If you started needing to call HostThreadXXX methods in generic code, then you were doing something wrong.
> 
> Jim
> 
> 
> > On Aug 28, 2014, at 2:51 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Hi majnemer, rnk, emaste, tfiala,
> >
> > This class implements a host thread for the different platforms.  As with HostProcess, no code has been updated to use HostThread yet, as this will clutter up the code review unnecessarily.  However, the methods of HostThread generally map to methods of Host, so the conversion should be mostly mechanical.  Because the actual conversion of client code has not been updated to use HostThread yet, it means the code in HostThread is largely duplicated.  This duplication will be removed after the transition takes place.
> >
> > I'm not super good at Linux, MacOSX, or FreeBSD, so I've included various people as reviewers in hopes of catching anything I may have implemented incorrectly.  But don't feel obligated to review everything.
> >
> > I've compiled and tested these changes on Mac, Linux, and Windows.  (note, however, since nobody has been updated to use HostThread yet, there's little to actually test).
> >
> > http://reviews.llvm.org/D5110
> >
> > Files:
> >  include/lldb/Core/DataBuffer.h
> >  include/lldb/Host/HostThread.h
> >  include/lldb/Host/freebsd/HostThreadFreeBSD.h
> >  include/lldb/Host/linux/HostThreadLinux.h
> >  include/lldb/Host/macosx/HostThreadMacOSX.h
> >  include/lldb/Host/posix/HostThreadPosix.h
> >  include/lldb/Host/windows/HostThreadWindows.h
> >  include/lldb/lldb-types.h
> >  lldb.xcodeproj/project.pbxproj
> >  source/Host/CMakeLists.txt
> >  source/Host/common/Host.cpp
> >  source/Host/freebsd/Host.cpp
> >  source/Host/freebsd/HostThreadFreeBSD.cpp
> >  source/Host/linux/Host.cpp
> >  source/Host/linux/HostThreadLinux.cpp
> >  source/Host/macosx/Host.mm
> >  source/Host/macosx/HostThreadMacOSX.cpp
> >  source/Host/posix/HostThreadPosix.cpp
> >  source/Host/windows/HostThreadWindows.cpp
> > <D5110.13058.patch>_______________________________________________
> > lldb-commits mailing list
> > lldb-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> 
>