Remove last Host usage from ArchSpec

[labath]

Summary

In 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.

[clayborg]

I just saw Jim's email for this, Please comment in this bug so we can all see the info in one location. Accepting pending the outcome of the discussion that Jim start in email that I am pasting below:

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

[zturner]

I'd be open to having another organizational component that isn't Utility. But as Jim said, there isn't a critical mass of stuff yet in Utility to figure out where it makes sense to draw the line.

In all honesty, that second component might just end up being "Core" again, if enough stuff can be moved out of Core that it doesn't have to depend on Symbol, Host, Interpreter, Breakpoint, etc.

BTW, I'm curious why you say that having Triple in llvm/ADT would be confusing. Isn't that where it already is? Or are you saying it's already confusing?

There's been some discussion on the LLVM side recently about how Support is just a catch-all for random stuff that doesn't really belong anywhere else, and there seems to be general support for separating some things out. I actually made a small effort towards that end several months ago, when I took all of the object file format stuff out of Support and made a new target called BinaryFormat out of it. But, it was easy to see what the best way to do that was because there was already a critical mass of stuff there, so it was easy to identify a logical grouping that could be separated.

[jingham]

Yes, my intention was not to gate this patch on figuring out the right role for Utility/Core/Whatever. It just seemed an apropos moment to bring this up.

You're right, the Triple stuff is in ADT! I was using it as an example of something you clearly wouldn't do so I didn't actually check to see if it was done. That's pretty funny.

I thought that ADT stood for Abstract Data Types - though I actually I don't remember where I got that impression... Having Triple there does seem confusing. That is a "how we specify Targets type thing", not a fancy data type type thing... I would never think to look for that facility alongside StringRef & DenseMap etc.

[probinson]

Drive by comment:

In D39896#922224, @jingham wrote:

You're right, the Triple stuff is in ADT! I was using it as an example of something you clearly wouldn't do so I didn't actually check to see if it was done. That's pretty funny.

I thought that ADT stood for Abstract Data Types - though I actually I don't remember where I got that impression... Having Triple there does seem confusing. That is a "how we specify Targets type thing", not a fancy data type type thing... I would never think to look for that facility alongside StringRef & DenseMap etc.

Triple.h is in ADT; Triple.cpp is in Support. I agree it belongs in Support, and I'm not sure why it's schizo like that.

[zturner]

In D39896#922381, @probinson wrote:

Drive by comment:

In D39896#922224, @jingham wrote:

You're right, the Triple stuff is in ADT! I was using it as an example of something you clearly wouldn't do so I didn't actually check to see if it was done. That's pretty funny.

I thought that ADT stood for Abstract Data Types - though I actually I don't remember where I got that impression... Having Triple there does seem confusing. That is a "how we specify Targets type thing", not a fancy data type type thing... I would never think to look for that facility alongside StringRef & DenseMap etc.

Triple.h is in ADT; Triple.cpp is in Support. I agree it belongs in Support, and I'm not sure why it's schizo like that.

Ok now that's just crazy. You're making me want to go fix this...

[labath]

If I understand correctly, everyone agrees with this change, so I will commit it soon(ish). Below are my thoughs on some of the things said in this thread.

@zturner wrote:

Super awesome. When you do move it to Utility, can you run the deps python script to see if any cmake dependencies can be updated?

The only change necessary was to *add* Utility as a dependency of the ArchitectureArm plugin, which did not depend (directly) on Utility before this.

In D39896#922381, @probinson wrote:

Drive by comment:

Triple.h is in ADT; Triple.cpp is in Support. I agree it belongs in Support, and I'm not sure why it's schizo like that.

That's because there is no llvm/lib/ADT, and there couldn't be one in the present form because of all the mutual dependencies between ADT and Support headers. So all of the ADT cpp files live in Support, but it's not so glaringly obvious as most of ADT is header-only. Triple is one of few headers that actually has a cpp file. (That said, I can see some reasoning behind it being in ADT, but I do think it would look better in Support.)

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.

In our distribution model, we copy the lldb-server to the phone the first time you start a debug session, so size is still important to us (we're not counting every kilobyte, but a megabyte more or less matters). liblldb is currently ~61 MB, while lldb-server (x86_64) is 9.6 (less, when you compile with -Os). Initially, the two numbers were nearly equal. Right now we're mostly happy with the size (although we wouldn't mind it being smaller), so size is not my primary motivation here.

In D39896#922134, @zturner wrote:

I'd be open to having another organizational component that isn't Utility. But as Jim said, there isn't a critical mass of stuff yet in Utility to figure out where it makes sense to draw the line.

In all honesty, that second component might just end up being "Core" again, if enough stuff can be moved out of Core that it doesn't have to depend on Symbol, Host, Interpreter, Breakpoint, etc.

I agree. At one point in not too distant future we will need to sit down and figure out how to split up Utility (otherwise, the layering problem can be vacuously "solved" by moving everything into a single layer). I don't think it will be easy though, as there is a natural tendency for the lowest layers of anything to become a collection of random unrelated tidbits (which is what happened to llvm support).

My ideas for the next steps are roughly as follows:

• move ProcessInfo class and friends to Utility or Host. This is so that the "get me the list of processes on this machine" functionality can work without including Process.h
• move the symbol-finding code from Host to Platform -- most of this is not host- but target-specific. E.g. the linux way of finding symbols could easily work for remote debugging from other OSes, if you had a suitable sysroot copied or network-mounted.
• move RegisterValue to Utility -- useful in lldb-server, unlike a lot of the stuff in Core.

lldb/trunk/include/lldb/Host/HostInfoBase.h

eArchKind32, // If this host supports 32 bit programs, return the default 32
                  // bit arch
     eArchKind64  // If this host supports 64 bit programs, return the default 64
                  // bit arch
   };
 
   static const ArchSpec &
   GetArchitecture(ArchitectureKind arch_kind = eArchKindDefault);
 
+  static llvm::Optional<ArchitectureKind> ParseArchitectureKind(llvm::StringRef kind);
+
   //------------------------------------------------------------------
   /// Find a resource files that are related to LLDB.
   ///
   /// Operating systems have different ways of storing shared
   /// libraries and related resources. This function abstracts the
   /// access to these paths.
   ///
   /// @param[in] path_type

lldb/trunk/source/Core/ArchSpec.cpp

 //===-- ArchSpec.cpp --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/ArchSpec.h"
 
-#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/Stream.h" // for Stream
 #include "lldb/Utility/StringList.h"
 #include "lldb/lldb-defines.h" // for LLDB_INVALID_C...
 #include "lldb/lldb-forward.h" // for RegisterContextSP
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h" // for Twine
bool ArchSpec::SetTriple(llvm::StringRef triple) {
   if (triple.empty()) {
     Clear();
     return false;
   }
 
   if (ParseMachCPUDashSubtypeTriple(triple, *this))
     return true;
 
-  if (triple.startswith(LLDB_ARCH_DEFAULT)) {
-    // Special case for the current host default architectures...
-    if (triple.equals(LLDB_ARCH_DEFAULT_32BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-    else if (triple.equals(LLDB_ARCH_DEFAULT_64BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind64);
-    else if (triple.equals(LLDB_ARCH_DEFAULT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-  } else {
-    SetTriple(llvm::Triple(llvm::Triple::normalize(triple)));
-  }
   return IsValid();
 }
 
 bool ArchSpec::ContainsOnlyArch(const llvm::Triple &normalized_triple) {
   return !normalized_triple.getArchName().empty() &&
          normalized_triple.getOSName().empty() &&
          normalized_triple.getVendorName().empty() &&
          normalized_triple.getEnvironmentName().empty();

lldb/trunk/source/Host/common/HostInfoBase.cpp

const ArchSpec &HostInfoBase::GetArchitecture(ArchitectureKind arch_kind) {
   if (arch_kind == eArchKind64)
     return g_fields->m_host_arch_64;
 
   // Otherwise prefer the 64-bit architecture if it is valid.
   return (g_fields->m_host_arch_64.IsValid()) ? g_fields->m_host_arch_64
                                               : g_fields->m_host_arch_32;
 }
 
+llvm::Optional<HostInfoBase::ArchitectureKind> HostInfoBase::ParseArchitectureKind(llvm::StringRef kind) {
+  return llvm::StringSwitch<llvm::Optional<ArchitectureKind>>(kind)
+      .Case(LLDB_ARCH_DEFAULT, eArchKindDefault)
+      .Case(LLDB_ARCH_DEFAULT_32BIT, eArchKind32)
+      .Case(LLDB_ARCH_DEFAULT_64BIT, eArchKind64)
+      .Default(llvm::None);
+}
+
 bool HostInfoBase::GetLLDBPath(lldb::PathType type, FileSpec &file_spec) {
   file_spec.Clear();
 
 #if defined(LLDB_DISABLE_PYTHON)
   if (type == lldb::ePathTypePythonDir)
     return false;
 #endif
 

 
 ArchSpec HostInfoBase::GetAugmentedArchSpec(llvm::StringRef triple) {
   if (triple.empty())
     return ArchSpec();
   llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
   if (!ArchSpec::ContainsOnlyArch(normalized_triple))
     return ArchSpec(triple);
 
+  if (auto kind = HostInfo::ParseArchitectureKind(triple))
+    return HostInfo::GetArchitecture(*kind);
+
   llvm::Triple host_triple(llvm::sys::getDefaultTargetTriple());
 
   if (normalized_triple.getVendorName().empty())
     normalized_triple.setVendor(host_triple.getVendor());
   if (normalized_triple.getOSName().empty())
     normalized_triple.setOS(host_triple.getOS());
   if (normalized_triple.getEnvironmentName().empty())
     normalized_triple.setEnvironment(host_triple.getEnvironment());

lldb/trunk/source/Target/Platform.cpp

 
 ArchSpec Platform::GetAugmentedArchSpec(llvm::StringRef triple) {
   if (triple.empty())
     return ArchSpec();
   llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
   if (!ArchSpec::ContainsOnlyArch(normalized_triple))
     return ArchSpec(triple);
 
+  if (auto kind = HostInfo::ParseArchitectureKind(triple))
+    return HostInfo::GetArchitecture(*kind);
+
   ArchSpec compatible_arch;
   ArchSpec raw_arch(triple);
   if (!IsCompatibleArchitecture(raw_arch, false, &compatible_arch))
     return raw_arch;
 
   if (!compatible_arch.IsValid())
     return ArchSpec(normalized_triple);
 

lldb/trunk/unittests/Host/HostInfoTest.cpp

-//===-- HostTest.cpp --------------------------------------------*- C++ -*-===//
+//===-- HostInfoTest.cpp ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/HostInfo.h"
+#include "lldb/lldb-defines.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 using namespace llvm;
 
 namespace {
 class HostInfoTest: public ::testing::Test {
   public:
TEST_F(HostInfoTest, GetAugmentedArchSpec) {
 
   // But if we specify only an arch, we should fill in the rest from the host.
   spec = HostInfo::GetAugmentedArchSpec("x86_64");
   Triple triple(sys::getDefaultTargetTriple());
   EXPECT_EQ(spec.GetTriple().getArch(), Triple::x86_64);
   EXPECT_EQ(spec.GetTriple().getOS(), triple.getOS());
   EXPECT_EQ(spec.GetTriple().getVendor(), triple.getVendor());
   EXPECT_EQ(spec.GetTriple().getEnvironment(), triple.getEnvironment());
+
+  // Test LLDB_ARCH_DEFAULT
+  EXPECT_EQ(HostInfo::GetAugmentedArchSpec(LLDB_ARCH_DEFAULT).GetTriple(),
+            HostInfo::GetArchitecture(HostInfo::eArchKindDefault).GetTriple());
 }