Modernize and simplify HostInfo::GetOSKernelDescription

[labath]

Summary

Replace bool+by-ref argument with llvm::Optional, and move the common
implementation into HostInfoPOSIX. Based on my (simple) experiment,
the uname and the sysctl approach return the same value on MacOS, so
there's no need for a mac-specific implementation of this functionality.

[JDevlieghere]

LGTM

[mgorny]

lldb/include/lldb/Host/posix/HostInfoPosix.h

25

Ok, I must be missing something but I don't see the implementation of this in this patch.

lldb/source/Target/Platform.cpp

495

I'd also change this prototype while at it ;-).

[labath]

add missing implementation

[labath]

lldb/include/lldb/Host/posix/HostInfoPosix.h

25

I forgot to amend the implementation into the patch. Thanks for catching that.

lldb/source/Target/Platform.cpp

495

A lot of this code is only built/tested on some platforms, so I wanted to do that as a followup -- to reduce the number of moving parts. I also did GetOSBuildString in two parts, and both parts managed to break something.

[mgorny]

lldb/source/Host/posix/HostInfoPosix.cpp

44 ↗

(On Diff #382048)

I'm somewhat surprised that you need to do that. I wonder if there was a real reason why it's done or just someone being overly careful.

[labath]

Remove memset

lldb/source/Host/posix/HostInfoPosix.cpp

44 ↗

(On Diff #382048)

I'm pretty sure it isn't necessary.

[mgorny]

LGTM

lldb/include/lldb/Host/freebsd/HostInfoFreeBSD.h

 #include "llvm/Support/VersionTuple.h"
 
 namespace lldb_private {
 
 class HostInfoFreeBSD : public HostInfoPosix {
 public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
 };
 }
 
 #endif

lldb/include/lldb/Host/linux/HostInfoLinux.h

class HostInfoLinux : public HostInfoPosix {
   friend class HostInfoBase;
 
 public:
   static void Initialize(SharedLibraryDirectoryHelper *helper = nullptr);
   static void Terminate();
 
   static llvm::VersionTuple GetOSVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
   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);
 };
 }
 
 #endif

lldb/include/lldb/Host/macosx/HostInfoMacOSX.h

 
 class HostInfoMacOSX : public HostInfoPosix {
   friend class HostInfoBase;
 
 public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::VersionTuple GetMacCatalystVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
   static FileSpec GetXcodeContentsDirectory();
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
   static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk);
 
   /// Shared cache utilities

lldb/include/lldb/Host/netbsd/HostInfoNetBSD.h

 #include "llvm/Support/VersionTuple.h"
 
 namespace lldb_private {
 
 class HostInfoNetBSD : public HostInfoPosix {
 public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
 };
 }
 
 #endif

lldb/include/lldb/Host/openbsd/HostInfoOpenBSD.h

 #include "llvm/Support/VersionTuple.h"
 
 namespace lldb_private {
 
 class HostInfoOpenBSD : public HostInfoPosix {
 public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
 };
 }
 
 #endif

lldb/include/lldb/Host/posix/HostInfoPosix.h

 class UserIDResolver;
 
 class HostInfoPosix : public HostInfoBase {
   friend class HostInfoBase;
 
 public:
   static size_t GetPageSize();
   static bool GetHostname(std::string &s);
+  static llvm::Optional<std::string> GetOSKernelDescription();
 
   static uint32_t GetUserID();
   static uint32_t GetGroupID();
   static uint32_t GetEffectiveUserID();
   static uint32_t GetEffectiveGroupID();
 
   static FileSpec GetDefaultShell();
 

lldb/include/lldb/Host/windows/HostInfoWindows.h

public:
   static void Initialize(SharedLibraryDirectoryHelper *helper = nullptr);
   static void Terminate();
 
   static size_t GetPageSize();
   static UserIDResolver &GetUserIDResolver();
 
   static llvm::VersionTuple GetOSVersion();
   static llvm::Optional<std::string> GetOSBuildString();
-  static bool GetOSKernelDescription(std::string &s);
+  static llvm::Optional<std::string> GetOSKernelDescription();
   static bool GetHostname(std::string &s);
   static FileSpec GetProgramFileSpec();
   static FileSpec GetDefaultShell();
 
   static bool GetEnvironmentVar(const std::string &var_name, std::string &var);
 
 private:
   static FileSpec m_program_filespec;
 };
 }
 
 #endif

lldb/source/Host/freebsd/HostInfoFreeBSD.cpp

llvm::Optional<std::string> HostInfoFreeBSD::GetOSBuildString() {
   size_t osrev_len = sizeof(osrev);
 
   if (::sysctl(mib, 2, &osrev, &osrev_len, NULL, 0) == 0)
     return llvm::formatv("{0,8:8}", osrev).str();
 
   return llvm::None;
 }
 
-bool HostInfoFreeBSD::GetOSKernelDescription(std::string &s) {
-  struct utsname un;
-
-  ::memset(&un, 0, sizeof(utsname));
-  s.clear();
-
-  if (uname(&un) < 0)
-    return false;
-
-  s.assign(un.version);
-
-  return true;
-}
-
 FileSpec HostInfoFreeBSD::GetProgramFileSpec() {
   static FileSpec g_program_filespec;
   if (!g_program_filespec) {
     int exe_path_mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, getpid()};
     char exe_path[PATH_MAX];
     size_t exe_path_size = sizeof(exe_path);
     if (sysctl(exe_path_mib, 4, exe_path, &exe_path_size, NULL, 0) == 0)
       g_program_filespec.SetFile(exe_path, FileSpec::Style::native);
   }
   return g_program_filespec;
 }

lldb/source/Host/linux/HostInfoLinux.cpp

llvm::Optional<std::string> HostInfoLinux::GetOSBuildString() {
   ::memset(&un, 0, sizeof(utsname));
 
   if (uname(&un) < 0)
     return llvm::None;
 
   return std::string(un.release);
 }
 
-bool HostInfoLinux::GetOSKernelDescription(std::string &s) {
-  struct utsname un;
-
-  ::memset(&un, 0, sizeof(utsname));
-  s.clear();
-
-  if (uname(&un) < 0)
-    return false;
-
-  s.assign(un.version);
-  return true;
-}
-
 llvm::StringRef HostInfoLinux::GetDistributionId() {
   assert(g_fields && "Missing call to Initialize?");
   // Try to run 'lbs_release -i', and use that response for the distribution
   // id.
   llvm::call_once(g_fields->m_distribution_once_flag, []() {
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST));
     LLDB_LOGF(log, "attempting to determine Linux distribution...");

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

llvm::Optional<std::string> HostInfoMacOSX::GetOSBuildString() {
   char cstr[PATH_MAX];
   size_t cstr_len = sizeof(cstr);
   if (::sysctl(mib, 2, cstr, &cstr_len, NULL, 0) == 0)
     return std::string(cstr, cstr_len);
 
   return llvm::None;
 }
 
-bool HostInfoMacOSX::GetOSKernelDescription(std::string &s) {
-  int mib[2] = {CTL_KERN, KERN_VERSION};
-  char cstr[PATH_MAX];
-  size_t cstr_len = sizeof(cstr);
-  if (::sysctl(mib, 2, cstr, &cstr_len, NULL, 0) == 0) {
-    s.assign(cstr, cstr_len);
-    return true;
-  }
-  s.clear();
-  return false;
-}
-
 static void ParseOSVersion(llvm::VersionTuple &version, NSString *Key) {
   @autoreleasepool {
     NSDictionary *version_info =
       [NSDictionary dictionaryWithContentsOfFile:
        @"/System/Library/CoreServices/SystemVersion.plist"];
     NSString *version_value = [version_info objectForKey: Key];
     const char *version_str = [version_value UTF8String];
     version.tryParse(version_str);

lldb/source/Host/netbsd/HostInfoNetBSD.cpp

llvm::Optional<std::string> HostInfoNetBSD::GetOSBuildString() {
   size_t osrev_len = sizeof(osrev);
 
   if (::sysctl(mib, 2, &osrev, &osrev_len, NULL, 0) == 0)
     return llvm::formatv("{0,10:10}", osrev).str();
 
   return llvm::None;
 }
 
-bool HostInfoNetBSD::GetOSKernelDescription(std::string &s) {
-  struct utsname un;
-
-  ::memset(&un, 0, sizeof(un));
-  s.clear();
-
-  if (::uname(&un) < 0)
-    return false;
-
-  s.assign(un.version);
-
-  return true;
-}
-
 FileSpec HostInfoNetBSD::GetProgramFileSpec() {
   static FileSpec g_program_filespec;
 
   if (!g_program_filespec) {
     static const int name[] = {
         CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME,
     };
     char path[MAXPATHLEN];

lldb/source/Host/openbsd/HostInfoOpenBSD.cpp

llvm::Optional<std::string> HostInfoOpenBSD::GetOSBuildString() {
   size_t osrev_len = sizeof(osrev);
 
   if (::sysctl(mib, 2, &osrev, &osrev_len, NULL, 0) == 0)
     return llvm::formatv("{0,8:8}", osrev).str();
 
   return llvm::None;
 }
 
-bool HostInfoOpenBSD::GetOSKernelDescription(std::string &s) {
-  struct utsname un;
-
-  ::memset(&un, 0, sizeof(utsname));
-  s.clear();
-
-  if (uname(&un) < 0)
-    return false;
-
-  s.assign(un.version);
-
-  return true;
-}
-
 FileSpec HostInfoOpenBSD::GetProgramFileSpec() {
   static FileSpec g_program_filespec;
   return g_program_filespec;
 }

lldb/source/Host/windows/HostInfoWindows.cpp

 llvm::Optional<std::string> HostInfoWindows::GetOSBuildString() {
   llvm::VersionTuple version = GetOSVersion();
   if (version.empty())
     return llvm::None;
 
   return "Windows NT " + version.getAsString();
 }
 
-bool HostInfoWindows::GetOSKernelDescription(std::string &s) {
-  llvm::Optional<std::string> build = GetOSBuildString();
-  s = build.getValueOr("");
-  return build.hasValue();
+llvm::Optional<std::string> HostInfoWindows::GetOSKernelDescription() {
+  return GetOSBuildString();
 }
 
 bool HostInfoWindows::GetHostname(std::string &s) {
   wchar_t buffer[MAX_COMPUTERNAME_LENGTH + 1];
   DWORD dwSize = MAX_COMPUTERNAME_LENGTH + 1;
   if (!::GetComputerNameW(buffer, &dwSize))
     return false;
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

#if defined(__APPLE__)
   }
 #endif
 
   if (llvm::Optional<std::string> s = HostInfo::GetOSBuildString()) {
     response.PutCString("os_build:");
     response.PutStringAsRawHex8(*s);
     response.PutChar(';');
   }
-  std::string s;
-  if (HostInfo::GetOSKernelDescription(s)) {
+  if (llvm::Optional<std::string> s = HostInfo::GetOSKernelDescription()) {
     response.PutCString("os_kernel:");
-    response.PutStringAsRawHex8(s);
+    response.PutStringAsRawHex8(*s);
     response.PutChar(';');
   }
 
+  std::string s;
 #if defined(__APPLE__)
 
 #if defined(__arm__) || defined(__arm64__) || defined(__aarch64__)
   // For iOS devices, we are connected through a USB Mux so we never pretend to
   // actually have a hostname as far as the remote lldb that is connecting to
   // this lldb-platform is concerned
   response.PutCString("hostname:");
   response.PutStringAsRawHex8("127.0.0.1");

lldb/source/Target/Platform.cpp

 }
 
 llvm::Optional<std::string> Platform::GetOSBuildString() {
   if (IsHost())
     return HostInfo::GetOSBuildString();
   return GetRemoteOSBuildString();
 }
 
 bool Platform::GetOSKernelDescription(std::string &s) {
-  if (IsHost())
-    return HostInfo::GetOSKernelDescription(s);
+  if (IsHost()) {
+    llvm::Optional<std::string> desc = HostInfo::GetOSKernelDescription();
+    s = desc.getValueOr("");
+    return desc.hasValue();
+  }
   return GetRemoteOSKernelDescription(s);
 }
 
 void Platform::AddClangModuleCompilationOptions(
     Target *target, std::vector<std::string> &options) {
   std::vector<std::string> default_compilation_options = {
       "-x", "c++", "-Xclang", "-nostdsysteminc", "-Xclang", "-nostdsysteminc"};