change debugserver to return an empty platform name for unknown platforms, instead of crashing

[jasonmolenda]

Summary

debugserver is sometimes run in bringup environments, where debugserver doesn't yet recognize its platform code. debugserver will crash shortly after when strlen() is run on the nullptr returned, creating a std::string.

Instead, have debugserver return an empty platform name back up to lldb. lldb is in a better position to handle a triple with a missing component and surface an error message to the user, instead of having debugserver cease to be running.

[jasonmolenda]

Ah, one minute on this one. Going over the testsuite, there's a change in behavior of TestMacCatalyst.py and I'm not positive if it's a bug or the test case being overly sensitive.

[jasonmolenda]

Update patch to also update the loop in HandlePacket_qProcessInfo() which iterates over the binaries, looking for a binary that declares its platform. The first binary in the list is dyld, and this binary does not have a platform, but the loop would get back an empty string and terminate its search. Update this to recognize an empty platform name and continue searching, to preserve its original intended behavior for qProcessInfo.

[tschuett]

How about an optional instead, if GetPlatformString is fallible.

[jasonmolenda]

In D136719#3884836, @tschuett wrote:

How about an optional instead, if GetPlatformString is fallible.

Ah, interesting idea. debugserver doesn't use any llvm, but I believe we're building with C++17 these days so I could do it with std::optional.

[tschuett]

In D136719#3885974, @jasonmolenda wrote:

In D136719#3884836, @tschuett wrote:

How about an optional instead, if GetPlatformString is fallible.

Ah, interesting idea. debugserver doesn't use any llvm, but I believe we're building with C++17 these days so I could do it with std::optional.

Yep.

[jasonmolenda]

Update MachProcess::GetPlatformString to return a std::optional platform string, if the platform enumeration is recognized. Update callers to handle the absence of a value appropriately.

[tschuett]

Looks great, but I would have expected std::optional<std::string> and a nullopt.

[jasonmolenda]

In D136719#3886254, @tschuett wrote:

Looks great, but I would have expected std::optional<std::string> and a nullopt.

Yeah I'm fine with that, it does seems more natural. In this case we're returning the address of a half-dozen const strings in the text section, so there wasn't any lifetime worries or anything, I left it as a char* after thinking about it for a few seconds. I'll update to use a std::string, this is not a hot code path, a small object allocation doesn't matter.

[jasonmolenda]

Update patch to pass std::string's in the optionals, and use std::nullopt. Thanks for all the feedback on this one, Thorsten.

[tschuett]

lldb/tools/debugserver/source/RNBRemote.cpp

6264

Nit: the has_value()is redundant. optional has boolean conversion.

6266

Maybe *platform works.

[jasonmolenda]

Incorporate Thorsten's suggestions for more idiomatic use of the std::optional result.

[JDevlieghere]

Nice use of std::optional. LGTM.

[JDevlieghere]

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

725

This is unused

lldb/tools/debugserver/source/DNB.h

 #include "DNBDefs.h"
 #include "JSONGenerator.h"
 #include "MacOSX/Genealogy.h"
 #include "MacOSX/ThreadInfo.h"
 #include "RNBContext.h"
 #include <Availability.h>
 #include <mach/machine.h>
 #include <mach/thread_info.h>
+#include <optional>
 #include <string>
 
 #define DNB_EXPORT __attribute__((visibility("default")))
 
 #ifndef CPU_TYPE_ARM64
 #define CPU_TYPE_ARM64 ((cpu_type_t)12 | 0x01000000)
 #endif
 
nub_thread_t DNBProcessGetThreadAtIndex(nub_process_t pid,
                                         nub_size_t thread_idx) DNB_EXPORT;
 nub_bool_t DNBProcessSyncThreadState(nub_process_t pid,
                                      nub_thread_t tid) DNB_EXPORT;
 nub_addr_t DNBProcessGetSharedLibraryInfoAddress(nub_process_t pid) DNB_EXPORT;
 nub_bool_t DNBProcessSharedLibrariesUpdated(nub_process_t pid) DNB_EXPORT;
 nub_size_t
 DNBProcessGetSharedLibraryInfo(nub_process_t pid, nub_bool_t only_changed,
                                DNBExecutableImageInfo **image_infos) DNB_EXPORT;
-const char *DNBGetDeploymentInfo(nub_process_t pid, bool is_executable,
-                                 const struct load_command &lc,
-                                 uint64_t load_command_address,
-                                 uint32_t &major_version,
-                                 uint32_t &minor_version,
-                                 uint32_t &patch_version);
+std::optional<std::string>
+DNBGetDeploymentInfo(nub_process_t pid, bool is_executable,
+                     uint64_t load_command_address, uint32_t &major_version,
+                     uint32_t &minor_version, uint32_t &patch_version);
 nub_bool_t DNBProcessSetNameToAddressCallback(nub_process_t pid,
                                               DNBCallbackNameToAddress callback,
                                               void *baton) DNB_EXPORT;
 nub_bool_t DNBProcessSetSharedLibraryInfoCallback(
     nub_process_t pid, DNBCallbackCopyExecutableImageInfos callback,
     void *baton) DNB_EXPORT;
 nub_addr_t DNBProcessLookupAddress(nub_process_t pid, const char *name,
                                    const char *shlib) DNB_EXPORT;

lldb/tools/debugserver/source/DNB.cpp

nub_bool_t DNBProcessSharedLibrariesUpdated(nub_process_t pid) {
   MachProcessSP procSP;
   if (GetProcessSP(pid, procSP)) {
     procSP->SharedLibrariesUpdated();
     return true;
   }
   return false;
 }
 
-const char *DNBGetDeploymentInfo(nub_process_t pid, bool is_executable,
-                                 const struct load_command &lc,
-                                 uint64_t load_command_address,
-                                 uint32_t &major_version,
-                                 uint32_t &minor_version,
-                                 uint32_t &patch_version) {
+std::optional<std::string>
+DNBGetDeploymentInfo(nub_process_t pid, bool is_executable,
+                     uint64_t load_command_address, uint32_t &major_version,
+                     uint32_t &minor_version, uint32_t &patch_version) {
   MachProcessSP procSP;
   if (GetProcessSP(pid, procSP)) {
     // FIXME: This doesn't return the correct result when xctest (a
     // macOS binary) is loaded with the macCatalyst dyld platform
     // override. The image info corrects for this, but qProcessInfo
     // will return what is in the binary.
     auto info =
         procSP->GetDeploymentInfo(lc, load_command_address, is_executable);

lldb/tools/debugserver/source/MacOSX/MachProcess.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TOOLS_DEBUGSERVER_SOURCE_MACOSX_MACHPROCESS_H
 #define LLDB_TOOLS_DEBUGSERVER_SOURCE_MACOSX_MACHPROCESS_H
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <mach-o/loader.h>
 #include <mach/mach.h>
+#include <optional>
 #include <pthread.h>
 #include <sys/signal.h>
 #include <uuid/uuid.h>
 #include <vector>
 
 #include "DNBBreakpoint.h"
 #include "DNBDefs.h"
 #include "DNBError.h"
struct DeploymentInfo {
     unsigned char platform = 0;
     uint32_t major_version = 0;
     uint32_t minor_version = 0;
     uint32_t patch_version = 0;
   };
   DeploymentInfo GetDeploymentInfo(const struct load_command &,
                                    uint64_t load_command_address,
                                    bool is_executable);
-  static const char *GetPlatformString(unsigned char platform);
+  static std::optional<std::string> GetPlatformString(unsigned char platform);
   bool GetMachOInformationFromMemory(uint32_t platform,
                                      nub_addr_t mach_o_header_addr,
                                      int wordsize,
                                      struct mach_o_information &inf);
   JSONGenerator::ObjectSP FormatDynamicLibrariesIntoJSON(
       const std::vector<struct binary_image_information> &image_infos);
   uint32_t GetPlatform();
   /// Get the runtime platform from DYLD via SPI.

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

if (minor && *minor == '.') {
       if (patch && *patch == '.')
         info.patch_version = std::strtoul(++patch, nullptr, 10);
     }
   }
 
   return info;
 }
 
-const char *MachProcess::GetPlatformString(unsigned char platform) {
+std::optional<std::string>
+MachProcess::GetPlatformString(unsigned char platform) {
   switch (platform) {
   case PLATFORM_MACOS:
     return "macosx";
   case PLATFORM_MACCATALYST:
     return "maccatalyst";
   case PLATFORM_IOS:
     return "ios";
   case PLATFORM_IOSSIMULATOR:
     return "iossimulator";
   case PLATFORM_TVOS:
     return "tvos";
   case PLATFORM_TVOSSIMULATOR:
     return "tvossimulator";
   case PLATFORM_WATCHOS:
     return "watchos";
   case PLATFORM_WATCHOSSIMULATOR:
     return "watchossimulator";
   case PLATFORM_BRIDGEOS:
     return "bridgeos";
   case PLATFORM_DRIVERKIT:
     return "driverkit";
+  default:
+    DNBLogError("Unknown platform %u found for one binary", platform);
+    return std::nullopt;
   }
-  return nullptr;
 }
 
 static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
   if (magic != MH_MAGIC && magic != MH_CIGAM && magic != MH_MAGIC_64 &&
       magic != MH_CIGAM_64)
     return false;
   if (cputype != CPU_TYPE_I386 && cputype != CPU_TYPE_X86_64 &&
       cputype != CPU_TYPE_ARM && cputype != CPU_TYPE_ARM64 &&
for (uint32_t j = 0; j < inf.mach_header.ncmds; j++) {
     if (lc.cmd == LC_UUID) {
       struct uuid_command uuidcmd;
       if (ReadMemory(load_cmds_p, sizeof(struct uuid_command), &uuidcmd) ==
           sizeof(struct uuid_command))
         uuid_copy(inf.uuid, uuidcmd.uuid);
     }
     if (DeploymentInfo deployment_info = GetDeploymentInfo(
             lc, load_cmds_p, inf.mach_header.filetype == MH_EXECUTE)) {
-      const char *lc_platform = GetPlatformString(deployment_info.platform);
+      std::optional<std::string> lc_platform =
+          GetPlatformString(deployment_info.platform);
       if (dyld_platform != PLATFORM_MACCATALYST &&
           inf.min_version_os_name == "macosx") {
         // macCatalyst support.
         //
         // This the special case of "zippered" frameworks that have both
         // a PLATFORM_MACOS and a PLATFORM_MACCATALYST load command.
         //
         // When we are in this block, this is a binary with both
         // PLATFORM_MACOS and PLATFORM_MACCATALYST load commands and
         // the process is not running as PLATFORM_MACCATALYST. Stick
         // with the "macosx" load command that we've already
         // processed, ignore this one, which is presumed to be a
         // PLATFORM_MACCATALYST one.
       } else {
-        inf.min_version_os_name = lc_platform;
+        inf.min_version_os_name = lc_platform.value_or("");
         inf.min_version_os_version = "";
         inf.min_version_os_version +=
             std::to_string(deployment_info.major_version);
         inf.min_version_os_version += ".";
         inf.min_version_os_version +=
             std::to_string(deployment_info.minor_version);
         if (deployment_info.patch_version != 0) {
           inf.min_version_os_version += ".";

lldb/tools/debugserver/source/RNBRemote.cpp

if (exe_mach_header_addr != INVALID_NUB_ADDRESS) {
       load_command lc;
       for (uint32_t i = 0; i < mh.ncmds && !os_handled; ++i) {
         const nub_size_t bytes_read =
             DNBProcessMemoryRead(pid, load_command_addr, sizeof(lc), &lc);
         (void)bytes_read;
 
         bool is_executable = true;
         uint32_t major_version, minor_version, patch_version;
-        auto *platform =
+        std::optional<std::string> platform =
             DNBGetDeploymentInfo(pid, is_executable, lc, load_command_addr,
                                  major_version, minor_version, patch_version);
         if (platform) {
           os_handled = true;
-          rep << "ostype:" << platform << ";";
+          rep << "ostype:" << *platform << ";";
           break;
         }
         load_command_addr = load_command_addr + lc.cmdsize;
       }
     }
 #endif // TARGET_OS_OSX
   }