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

[Zachary Turner]

On Thu, Aug 28, 2014 at 7:58 PM, <jingham at apple.com> wrote:

> 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...
>
I'm not saying you have to, I'm saying there's no reason not to, because
for any given build configuration, there will just be 1 HostThread class.
 What's the point of passing around a HostThreadBase everywhere, when the
only thing that derives from it is a HostThreadMacOSX?


> >
> > 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...
>
You probably wouldn't need it, although I can imagine some situations.
 These are not thread-related mind you, but off the top of my head, the
public API currently exposes some methods that allow you to get the
effective user id, group id, etc of the process.  I don't want a method on
HostInfoWindows called GetEffectiveUserId() that returns 0.  It's a concept
that doesn't exist.  Likewise, you probably don't want a method on
HostInfoMacOSX called GetCurrentUserDomainName().  Instead, in these
exceptional cases where there's a public API that does platform specific
stuff, I'm just saying you can move the pre-processor decision there.

Just to be clear though, there would not be any code that looks like this:

#if HOST_THREAD == HostThreadMacOSX
#endif

it would be this:

#if defined(__APPLE__)
   thread.SomeAppleSpecificMethod();
#endif

This kind of code is already fairly prevalent in the codebase, and I don't
anticipate that my changes as proposed will increase this kind of thing at
all.  Because even under the solution you're proposing with using
HostThreadBase everywhere, it's still possible, but like this:

#if defined(__APPLE__)
   ((HostThreadMacOSX&)thread).SomeAppleSpecificMethod();
#endif

It's the same thing, but just uglier.

FWIW, I think we are on the same page about wanting to keep pre-processor
defines to a minimum in the codebase.  That's actually a large part of my
motivation for doing all this refactoring in the first place.  If you take
a look at the various HostInfo classes, that refactor removed almost 100%
of preprocessor definitions that were in the original code.  So I'm very
cognizant of them and one of my explicit goals is getting rid of them.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140828/9245805f/attachment.html>