[Lldb-commits] [PATCH] Implements a HostThread class.
zturner at google.com
Thu Aug 28 20:53:43 PDT 2014
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
>> > 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
>> > 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
> it would be this:
> #if defined(__APPLE__)
> 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__)
> 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...
More information about the lldb-commits