[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

[Jim Ingham via lldb-commits]

I'm not sure we have enough instances to decide on better organization, but ArchSpec really doesn't feel like a Utility for lldb.  That would be like moving the llvm triple handling to ADT, that seems a little weird.  Similarly having the register stuff in Utility seems odd as well.  I would never think to look for it there.  Pavel asked me a while ago to talk a bit about what the strategy for the directories in lldb was originally and I started to answer and then got sidetracked by events and never answered.  

The relevant historical bit for this discussion was originally the directory layout had nothing to do with building (Xcode doesn't care about files being in different directories, and we didn't envision building pieces of lldb separately at that point.  I did it originally with the sole intent making an organization for people new to the code to find and remember where files were.  We weren't super-rigorous about this - particularly as Xcode got better at finding files for you so finding them in directories was less important on our end.  So for the purposes of reflecting code organization, we could probably stand another round of organization to make the structure more obvious.  But I do think that's still a worthy goal.

Anyway, then just because of the way cmake works (or is used in llvm I don't actually know which) we've switch the meaning of the directories to "files that are built into a .a file".  And then, because of the need to shrink the code size of lldb-server, it became important to make at least the lower-level stuff that lldb-server depended on independent of as much of the rest of lldb as possible.  So it became important for at least some directories to contain "files that are built into .a files that don't depend on some/most of the other .a files."  That's introduced a somewhat orthogonal design principle for the directory layout.

Note, Greg and I used to argue about the strategy for lldb-server.  My notion was on modern systems the actual file size difference between an lldb-server that used all of lldb.framework, and one that could use a cut down library was really not all that important.  If you're making a stub for an hard embedded system, you probably aren't going to use lldb-server, you'll use a much smaller gdb-protocol stub, and we didn't really have the intent to provide that functionality.  So lldb server as intended for things like phones etc, where "small" means small in modern terms, not "a kilobyte matters" type small. 

I always felt the appropriate discipline to impose was making sure that if you didn't use something in lldb (symbols for instance) you don't pay memory/time cost for it.  If that was true, and the bigger size was not an issue, than having lldb-server built with the full lldb.framework would mean that you would have flexibility to move work from the host to the lldb-server end of a remote debugging session, which could be really convenient since you could offload work to the remote end w/o having to come back over the slower remote communication channel.  We never really convinced on another either way and since he directed most of the development of lldb-server on our end, he won by dint of implementation.  But that's why for me the breakup of LLDB.framework into independent pieces was never a particularly present goal.

Any, that's water under the bridge at this point.  But I'd also like not to lose the benefit of comprehension that was the original reason for the directory layout.  And in this instance, ArchSpec really doesn't seem like a Utility, which brought this back up in my thinking.  There doesn't seem a critical mass of files in Utility to make decisions at this point, but as we break up Core, it might be nice to have a place for things that aren't Support or ADT type stuff, but more the core business of a debugger, but also don't drag in Symbol and other parts of lldb that aren't appropriate for lldb-server.

Jim



> On Nov 10, 2017, at 4:02 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> labath created this revision.
> 
> In https://reviews.llvm.org/D39387, I was quick to jump to conclusion that ArchSpec has no
> external dependencies. It turns there still was one call to
> HostInfo::GetArchitecture left -- for implementing the "systemArch32"
> architecture and friends.
> 
> Since GetAugmentedArchSpec is the place we handle these "incomplete"
> triples that don't specify os or vendor and "systemArch" looks very much
> like an incomplete triple, I move its handling there.
> 
> After this ArchSpec *really* does not have external dependencies, and
> I'd like to move it to the Utility module as a follow-up.
> 
> 
> https://reviews.llvm.org/D39896
> 
> Files:
>  include/lldb/Host/HostInfoBase.h
>  source/Core/ArchSpec.cpp
>  source/Host/common/HostInfoBase.cpp
>  source/Target/Platform.cpp
>  unittests/Host/HostInfoTest.cpp
> 
> <D39896.122416.patch>