[lldb][Linux] Moving generic APIs from HostInfoLinux to HostInfoPosix

[DhruvSrivastavaX]

This change is related merging some of the APIs in HostInfoLinux into HostInfoPosix.

Here is the reference PR comment:
#117906 (comment), #117906 (comment)

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dhruv Srivastava (DhruvSrivastavaX)

Changes

This change is related merging some of the APIs in HostInfoLinux into HostInfoPosix.
Here is the reference PR comment:
#117906 (comment), #117906 (comment)

The overall change might have some gaps that need to be filled and may cause some test failures.
But please let me know if this is how you had it in mind?

@labath @DavidSpickett

---

Full diff: https://github.com/llvm/llvm-project/pull/119694.diff

4 Files Affected:

• (modified) lldb/include/lldb/Host/linux/HostInfoLinux.h (+1-8)
• (modified) lldb/include/lldb/Host/posix/HostInfoPosix.h (+7-1)
• (modified) lldb/source/Host/linux/HostInfoLinux.cpp (+4-58)
• (modified) lldb/source/Host/posix/HostInfoPosix.cpp (+85-2)

diff --git a/lldb/include/lldb/Host/linux/HostInfoLinux.h b/lldb/include/lldb/Host/linux/HostInfoLinux.h
index 2964f3f1dc9893..1d6a1c3b0412a0 100644
--- a/lldb/include/lldb/Host/linux/HostInfoLinux.h
+++ b/lldb/include/lldb/Host/linux/HostInfoLinux.h
@@ -15,7 +15,6 @@
 #include "llvm/Support/VersionTuple.h"
 
 #include <optional>
-#include <string>
 
 namespace lldb_private {
 
@@ -26,18 +25,12 @@ class HostInfoLinux : public HostInfoPosix {
   static void Initialize(SharedLibraryDirectoryHelper *helper = nullptr);
   static void Terminate();
 
-  static llvm::VersionTuple GetOSVersion();
-  static std::optional<std::string> GetOSBuildString();
   static llvm::StringRef GetDistributionId();
-  static FileSpec GetProgramFileSpec();
 
 protected:
-  static bool ComputeSupportExeDirectory(FileSpec &file_spec);
-  static bool ComputeSystemPluginsDirectory(FileSpec &file_spec);
-  static bool ComputeUserPluginsDirectory(FileSpec &file_spec);
   static void ComputeHostArchitectureSupport(ArchSpec &arch_32,
                                              ArchSpec &arch_64);
 };
-}
+} // namespace lldb_private
 
 #endif
diff --git a/lldb/include/lldb/Host/posix/HostInfoPosix.h b/lldb/include/lldb/Host/posix/HostInfoPosix.h
index 8d070d3ac1e6fe..30f67515f35e23 100644
--- a/lldb/include/lldb/Host/posix/HostInfoPosix.h
+++ b/lldb/include/lldb/Host/posix/HostInfoPosix.h
@@ -12,6 +12,7 @@
 #include "lldb/Host/HostInfoBase.h"
 #include "lldb/Utility/FileSpec.h"
 #include <optional>
+#include <string>
 
 namespace lldb_private {
 
@@ -31,15 +32,20 @@ class HostInfoPosix : public HostInfoBase {
   static uint32_t GetEffectiveGroupID();
 
   static FileSpec GetDefaultShell();
+  static FileSpec GetProgramFileSpec();
 
   static bool GetEnvironmentVar(const std::string &var_name, std::string &var);
 
   static UserIDResolver &GetUserIDResolver();
+  static llvm::VersionTuple GetOSVersion();
+  static std::optional<std::string> GetOSBuildString();
 
 protected:
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);
+  static bool ComputeSystemPluginsDirectory(FileSpec &file_spec);
+  static bool ComputeUserPluginsDirectory(FileSpec &file_spec);
 };
-}
+} // namespace lldb_private
 
 #endif
diff --git a/lldb/source/Host/linux/HostInfoLinux.cpp b/lldb/source/Host/linux/HostInfoLinux.cpp
index 723f0c2fb3fdc6..ed8ac614ee02b8 100644
--- a/lldb/source/Host/linux/HostInfoLinux.cpp
+++ b/lldb/source/Host/linux/HostInfoLinux.cpp
@@ -30,8 +30,8 @@ namespace {
 struct HostInfoLinuxFields {
   llvm::once_flag m_distribution_once_flag;
   std::string m_distribution_id;
-  llvm::once_flag m_os_version_once_flag;
-  llvm::VersionTuple m_os_version;
+  // llvm::once_flag m_os_version_once_flag;
+  // llvm::VersionTuple m_os_version;
 };
 } // namespace
 
@@ -50,7 +50,7 @@ void HostInfoLinux::Terminate() {
   HostInfoBase::Terminate();
 }
 
-llvm::VersionTuple HostInfoLinux::GetOSVersion() {
+/*llvm::VersionTuple HostInfoPosix::GetOSVersion() {
   assert(g_fields && "Missing call to Initialize?");
   llvm::call_once(g_fields->m_os_version_once_flag, []() {
     struct utsname un;
@@ -66,17 +66,7 @@ llvm::VersionTuple HostInfoLinux::GetOSVersion() {
 
   return g_fields->m_os_version;
 }
-
-std::optional<std::string> HostInfoLinux::GetOSBuildString() {
-  struct utsname un;
-  ::memset(&un, 0, sizeof(utsname));
-
-  if (uname(&un) < 0)
-    return std::nullopt;
-
-  return std::string(un.release);
-}
-
+*/
 llvm::StringRef HostInfoLinux::GetDistributionId() {
   assert(g_fields && "Missing call to Initialize?");
   // Try to run 'lbs_release -i', and use that response for the distribution
@@ -152,50 +142,6 @@ llvm::StringRef HostInfoLinux::GetDistributionId() {
   return g_fields->m_distribution_id;
 }
 
-FileSpec HostInfoLinux::GetProgramFileSpec() {
-  static FileSpec g_program_filespec;
-
-  if (!g_program_filespec) {
-    char exe_path[PATH_MAX];
-    ssize_t len = readlink("/proc/self/exe", exe_path, sizeof(exe_path) - 1);
-    if (len > 0) {
-      exe_path[len] = 0;
-      g_program_filespec.SetFile(exe_path, FileSpec::Style::native);
-    }
-  }
-
-  return g_program_filespec;
-}
-
-bool HostInfoLinux::ComputeSupportExeDirectory(FileSpec &file_spec) {
-  if (HostInfoPosix::ComputeSupportExeDirectory(file_spec) &&
-      file_spec.IsAbsolute() && FileSystem::Instance().Exists(file_spec))
-    return true;
-  file_spec.SetDirectory(GetProgramFileSpec().GetDirectory());
-  return !file_spec.GetDirectory().IsEmpty();
-}
-
-bool HostInfoLinux::ComputeSystemPluginsDirectory(FileSpec &file_spec) {
-  FileSpec temp_file("/usr/" LLDB_INSTALL_LIBDIR_BASENAME "/lldb/plugins");
-  FileSystem::Instance().Resolve(temp_file);
-  file_spec.SetDirectory(temp_file.GetPath());
-  return true;
-}
-
-bool HostInfoLinux::ComputeUserPluginsDirectory(FileSpec &file_spec) {
-  // XDG Base Directory Specification
-  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html If
-  // XDG_DATA_HOME exists, use that, otherwise use ~/.local/share/lldb.
-  const char *xdg_data_home = getenv("XDG_DATA_HOME");
-  if (xdg_data_home && xdg_data_home[0]) {
-    std::string user_plugin_dir(xdg_data_home);
-    user_plugin_dir += "/lldb";
-    file_spec.SetDirectory(user_plugin_dir.c_str());
-  } else
-    file_spec.SetDirectory("~/.local/share/lldb");
-  return true;
-}
-
 void HostInfoLinux::ComputeHostArchitectureSupport(ArchSpec &arch_32,
                                                    ArchSpec &arch_64) {
   HostInfoPosix::ComputeHostArchitectureSupport(arch_32, arch_64);
diff --git a/lldb/source/Host/posix/HostInfoPosix.cpp b/lldb/source/Host/posix/HostInfoPosix.cpp
index 731a7dee2e6203..8a4a094db7b76f 100644
--- a/lldb/source/Host/posix/HostInfoPosix.cpp
+++ b/lldb/source/Host/posix/HostInfoPosix.cpp
@@ -7,16 +7,19 @@
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/posix/HostInfoPosix.h"
+#include "lldb/Host/Config.h"
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/UserIDResolver.h"
-
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <climits>
+#include <cstdio>
 #include <cstdlib>
+#include <cstring>
 #include <grp.h>
 #include <mutex>
 #include <optional>
@@ -27,6 +30,31 @@
 
 using namespace lldb_private;
 
+namespace {
+struct HostInfoPosixFields {
+  llvm::once_flag m_os_version_once_flag;
+  llvm::VersionTuple m_os_version;
+};
+} // namespace
+
+llvm::VersionTuple HostInfoPosix::GetOSVersion() {
+  static HostInfoPosixFields *g_fields = new HostInfoPosixFields();
+  assert(g_fields && "Missing call to Initialize?");
+  llvm::call_once(g_fields->m_os_version_once_flag, []() {
+    struct utsname un;
+    if (uname(&un) != 0)
+      return;
+
+    llvm::StringRef release = un.release;
+    // The kernel release string can include a lot of stuff (e.g.
+    // 4.9.0-6-amd64). We're only interested in the numbered prefix.
+    release = release.substr(0, release.find_first_not_of("0123456789."));
+    g_fields->m_os_version.tryParse(release);
+  });
+
+  return g_fields->m_os_version;
+}
+
 size_t HostInfoPosix::GetPageSize() { return ::getpagesize(); }
 
 bool HostInfoPosix::GetHostname(std::string &s) {
@@ -47,6 +75,16 @@ std::optional<std::string> HostInfoPosix::GetOSKernelDescription() {
   return std::string(un.version);
 }
 
+std::optional<std::string> HostInfoPosix::GetOSBuildString() {
+  struct utsname un;
+  ::memset(&un, 0, sizeof(utsname));
+
+  if (uname(&un) < 0)
+    return std::nullopt;
+
+  return std::string(un.release);
+}
+
 #ifdef __ANDROID__
 #include <android/api-level.h>
 #endif
@@ -139,8 +177,53 @@ FileSpec HostInfoPosix::GetDefaultShell() {
   return FileSpec("/bin/sh");
 }
 
+FileSpec HostInfoPosix::GetProgramFileSpec() {
+  static FileSpec g_program_filespec;
+
+  if (!g_program_filespec) {
+    char exe_path[PATH_MAX];
+    ssize_t len = readlink("/proc/self/exe", exe_path, sizeof(exe_path) - 1);
+    if (len > 0) {
+      exe_path[len] = 0;
+      g_program_filespec.SetFile(exe_path, FileSpec::Style::native);
+    }
+  }
+
+  return g_program_filespec;
+}
+
+// Keeping the original one for reference
+// bool HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec) {
+//  return ComputePathRelativeToLibrary(file_spec, "/bin");
+// }
+
 bool HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec) {
-  return ComputePathRelativeToLibrary(file_spec, "/bin");
+  if (ComputePathRelativeToLibrary(file_spec, "/bin") &&
+      file_spec.IsAbsolute() && FileSystem::Instance().Exists(file_spec))
+    return true;
+  file_spec.SetDirectory(GetProgramFileSpec().GetDirectory());
+  return !file_spec.GetDirectory().IsEmpty();
+}
+
+bool HostInfoPosix::ComputeSystemPluginsDirectory(FileSpec &file_spec) {
+  FileSpec temp_file("/usr/" LLDB_INSTALL_LIBDIR_BASENAME "/lldb/plugins");
+  FileSystem::Instance().Resolve(temp_file);
+  file_spec.SetDirectory(temp_file.GetPath());
+  return true;
+}
+
+bool HostInfoPosix::ComputeUserPluginsDirectory(FileSpec &file_spec) {
+  // XDG Base Directory Specification
+  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html If
+  // XDG_DATA_HOME exists, use that, otherwise use ~/.local/share/lldb.
+  const char *xdg_data_home = getenv("XDG_DATA_HOME");
+  if (xdg_data_home && xdg_data_home[0]) {
+    std::string user_plugin_dir(xdg_data_home);
+    user_plugin_dir += "/lldb";
+    file_spec.SetDirectory(user_plugin_dir.c_str());
+  } else
+    file_spec.SetDirectory("~/.local/share/lldb");
+  return true;
 }
 
 bool HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec) {

[DavidSpickett]

At a glance, this seems like what Pavel (and maybe me, I forget :) ) asked for.

Assuming you have a Linux system to test on, please get this to a reviewable point. Remove commented code, all that stuff. If the tests still pass, you should be fine.

Then it'll be much easier for us to see what is a simple move and what changed.

[labath]

I agree with David except for the GetProgramFileSpec thingy -- where I have a question.

[DhruvSrivastavaX]

The changes are done. Please let me know if it is okay now. @labath @DavidSpickett

[DavidSpickett]

Just need to remove the commented code and then I'm happy with this. I also edited your PR description to be ready for merging.

@labath should confirm that their final comment was addressed and approve if so.

[DhruvSrivastavaX]

Sure, Done with the changes. Thanks!

[labath]

Thanks.