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

[Zachary Turner]

FWIW, this is the same method I used for HostInfo, which is already in, so
you can search and see how that turned out with regards to preprocessor
defines.  A key point though, which maybe is already clear to you but I'll
re-iterate just in case, is that you don't ever actually write
HostInfoWindows.  In HostInfo.h, the most-derived type is typedefed to
HostInfo.  No matter where you are, whether it be generic LLDB code or
platform specific LLDB code, you just write HostInfo::method().

The same idea applies here.  You don't ever instantiate a HostThreadMacOSX
or a HostThreadWindows.  You instantiate a HostThread, and what type that
is depends on the platform.


On Thu, Aug 28, 2014 at 8:38 PM, Zachary Turner <zturner at google.com> wrote:

>
>
>
> 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/3286b513/attachment.html>