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

[jingham at apple.com]

I guess I view this the other way round from you.  

The host layer has two sides to it.  To me the primary one is to provide an interface that isolates generic lldb code from the underlying details of the host system that it is compiled for.  Secondarily it will provide interfaces that the various bits of the host layer for that particular host use to implement the generic Host layer.

It seems to me you are saying the job of providing a generic host layer for lldb to use is too difficult and so when generic lldb code is using the host layer to make a new thread, or manipulate a file, or launch a process, it has really got to know it is using host specific functionality whose specific details it might at any moment have to be aware of.  That just seems wrong to me.  If  I'm using some Host provided capability in Process, or ProcessGDBRemote or when dealing with FileSpec's anywhere in the Dwarf reading code or whatever I can't have to know what host I'm actually on.  Otherwise you're going to have to start intruding host specific details into generic lldb algorithms.

So it seems to me precisely the opposite of what you are claiming, if I have to have my hands on the Host specific version of any of the Host classes we are providing in Process, etc, then the host layer is a failed abstraction.  I don't think that is necessary...

> On Aug 28, 2014, at 7:07 PM, Zachary Turner <zturner at google.com> wrote:
> 
> 
> 
> 
> On Thu, Aug 28, 2014 at 5:50 PM, <jingham at apple.com> wrote:
> 
> > 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.
>  
> I agree that the performance overhead of virtual methods isn't significant.  On the other hand, it feels strange to use a language feature which changes the way code is generated just for the purposes of documentation.  That's usually what comments are for.  That said, I don't feel that strongly about it, so I'm willing to make a base class with a common interface.  It's surprisingly difficult to make a useful common interface for a thread though, because even methods that seem to make logical sense on all platforms are sometimes semantically different enough that it's difficult to find a function signature that has the stuff you need on every platform.  But it's worth trying.

> One thing I do feel kind of strongly about though is the idea of accessing this through a base class pointer or reference.  Who wants to deal with a base class when you can have the derived class?  It introduces extra, error-prone lines of code for no practical benefit.


I don't understand this comment.  In generic code, like Process or ProcessGDBRemote where we need to spawn a thread to handle the private process events or the gdb-remote reading, or start up debugserver, why would I need full HostThreadXXX or HostProcessXXX object?  The only reason would be because I'm going to have to do some:

#if HOST_THREAD == HostThreadXXX

stuff in generic code, but that means again the host abstraction has failed me.  I don't want that...

>  
> 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. 
>  
> Why would this ever happen?  If you're implementing a method of a Host specific class and you get handed a Host object, it is necessarily the Host object for the same platform as the other object you're implementing.  There's no such thing as an xxxWindows when you're on Mac.  You'll necessarily have an xxxMacOSX (or an xxxPosix, or whatever else is the most derived version of xxx for MacOSX).  Every host object that is passed around anywhere will always just be the exact type you need for your current platform.  This means you might break a compile on another platform, but in my opinion that's actually better than the alternative of providing a default method that does nothing, or asserts, because it forces you to think about whether you really have the right abstraction.  Additionally, if someone were porting to a different platform, they will know just by compiling LLDB on their new platform exactly the areas where they need to fix up, because the compile will break, as opposed to just running into strange runtime bugs that they have to track down and find out that there's some method that just returns NULL (and then shortly after, find out that the entire method is something that doesn't even make sense on their platform).

I guess I want the abstract part of the host layer to work well enough that you only have to use non-generic parts of it for really gnarly stuff.  So for the most part, unless you are implementing methods of the Host specific sub-class of some Host feature, you can use the generic interface.

>  
> 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...
> Agree with this, but I think the solution I'm proposing already solves that, albeit in a slightly different way.  If you use something that is too specific for the area of code that you're working in (e.g. call process.LaunchXPC or whatever) the compile will just break on other platforms.

I worry that in fact what is going to happen is that that is it is going to lead to more #if HOST_BLAH == HostBlahXXX in generic code, rather than in figuring out how to make a good host abstraction...

Jim