[lldb] Change ObjectValueDictionary to use a StringMap

[bulbazord]

Summary

llvm has a structure for maps where the key's type is a string. Using
that also means that the keys for OptionValueDictionary don't stick
around forever in ConstString's StringPool (even after they are gone).

The only thing we lose here is ordering: iterating over the map where the keys
are ConstStrings guarantees that we iterate in alphabetical order.
StringMap makes no guarantees about the ordering when you iterate over
the entire map.

[jingham]

No reason for these strings to be in the ConstString pool, so that part is fine.

But if we're going to use this to store things like the env-vars, having no ordering guarantees when we dump them seems likely to be a bit annoying. If the order of element output in settings show env-vars dump changes around every time I add an element to it, that would be make the results hard to follow. Can we make the dumper for this option value pull the keys out, alphabetize the keys, then look the values up in that order? I don't think printing these objects is ever going to be performance critical.

lldb/source/Core/Disassembler.cpp

846 ↗

(On Diff #518041)

That's kind of answering a question with a question: Why isn't there an eTypeUInt32?

lldb/source/Interpreter/OptionValueDictionary.cpp

39 ↗

(On Diff #518041)

Does the llvm::StringMap dingus not support for (auto value : m_values) { ?

[bulbazord]

lldb/source/Core/Disassembler.cpp

846 ↗

(On Diff #518041)

I'm not sure, we'd have to ask Caroline who added this back in 2011. I added a comment to help me remember, but I suppose this comment may add more confusion than remove. I'll remove the note.

lldb/source/Interpreter/OptionValueDictionary.cpp

39 ↗

(On Diff #518041)

Oh, it probably does. Didn't even think about it, good catch.

[JDevlieghere]

+1 on making the output deterministic with a sort

[jingham]

I think something went wrong here. This was a patch for OptionValueDictionary, but it seems to have become a patch for OpenInExternalEditor?

[JDevlieghere]

That's my bad: I put the wrong differential URL in my commit (13dbc16b4d82b9adc98c0919873b2b59ccc46afd). The correct differential revision for this was https://reviews.llvm.org/D149472. Sorry Alex!

[bulbazord]

Sort output for dumping to be deterministic (based on alphabetical order like previous implementation)

[JDevlieghere]

LGTM

lldb/source/Core/Disassembler.cpp

805 ↗

(On Diff #518542)

I don't think you need this: key is a std::string that can be used directly.

[bulbazord]

Remove unneeded StringRef

lldb/include/lldb/Host/Host.h

public:
   /// \arg hide_stderr If this is \b false, redirect stderr to stdout
   static Status RunShellCommand(llvm::StringRef shell, const Args &args,
                                 const FileSpec &working_dir, int *status_ptr,
                                 int *signo_ptr, std::string *command_output,
                                 const Timeout<std::micro> &timeout,
                                 bool run_in_shell = true,
                                 bool hide_stderr = false);
 
-  static bool OpenFileInExternalEditor(const FileSpec &file_spec,
+  static llvm::Error OpenFileInExternalEditor(const FileSpec &file_spec,
                                               uint32_t line_no);
 
   /// Check if we're running in an interactive graphical session.
   ///
   /// \return
   ///     True if we're running in an interactive graphical session. False if
   ///     we're not or don't know.
   static bool IsInteractiveGraphicSession();
 

lldb/source/Host/common/Host.cpp

 #endif // !defined(__APPLE__)
 
 #ifndef _WIN32
 void Host::Kill(lldb::pid_t pid, int signo) { ::kill(pid, signo); }
 
 #endif
 
 #if !defined(__APPLE__)
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
                                            uint32_t line_no) {
-  return false;
+  return llvm::errorCodeToError(
+      std::error_code(ENOTSUP, std::system_category()));
 }
 
 bool Host::IsInteractiveGraphicSession() { return false; }
 #endif
 
 std::unique_ptr<Connection> Host::CreateDefaultConnection(llvm::StringRef url) {
 #if defined(_WIN32)
   if (url.startswith("file://"))

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

LaunchInNewTerminalWithAppleScript(const char *exe_path,
   [applescript release];
   if (pid != LLDB_INVALID_PROCESS_ID)
     launch_info.SetProcessID(pid);
   return error;
 }
 
 #endif // TARGET_OS_OSX
 
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
                                            uint32_t line_no) {
 #if !TARGET_OS_OSX
-  return false;
+  return llvm::errorCodeToError(
+      std::error_code(ENOTSUP, std::system_category()));
 #else // !TARGET_OS_OSX
-  // We attach this to an 'odoc' event to specify a particular selection
+  Log *log = GetLog(LLDBLog::Host);
+
+  const std::string file_path = file_spec.GetPath();
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor",
+           file_path.empty() ? "<invalid>" : file_path, line_no);
+
+  if (file_path.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "no file specified");
+
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser<CFURLRef> file_URL = ::CFURLCreateWithFileSystemPath(
+      /*allocator=*/NULL,
+      /*filePath*/ file_cfstr.get(),
+      /*pathStyle=*/kCFURLPOSIXPathStyle,
+      /*isDirectory=*/false);
+
+  if (!file_URL.get())
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        llvm::formatv("could not create CFURL from path \"{0}\"", file_path));
+
+  // Create a new Apple Event descriptor.
   typedef struct {
     int16_t reserved0; // must be zero
     int16_t fLineNumber;
     int32_t fSelStart;
     int32_t fSelEnd;
     uint32_t reserved1; // must be zero
     uint32_t reserved2; // must be zero
   } BabelAESelInfo;
 
-  Log *log = GetLog(LLDBLog::Host);
-  char file_path[PATH_MAX];
-  file_spec.GetPath(file_path, PATH_MAX);
-  CFCString file_cfstr(file_path, kCFStringEncodingUTF8);
-  CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
-      NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
-
-  LLDB_LOGF(log,
-            "Sending source file: \"%s\" and line: %d to external editor.\n",
-            file_path, line_no);
-
-  long error;
+  // We attach this to an 'odoc' event to specify a particular selection.
   BabelAESelInfo file_and_line_info = {
       0,                      // reserved0
       (int16_t)(line_no - 1), // fLineNumber (zero based line number)
       1,                      // fSelStart
       1024,                   // fSelEnd
       0,                      // reserved1
       0                       // reserved2
   };
 
   AEKeyDesc file_and_line_desc;
+  file_and_line_desc.descKey = keyAEPosition;
+  long error = ::AECreateDesc(/*typeCode=*/typeUTF8Text,
+                              /*dataPtr=*/&file_and_line_info,
+                              /*dataSize=*/sizeof(file_and_line_info),
+                              /*result=*/&(file_and_line_desc.descContent));
+
+  if (error != noErr)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        llvm::formatv("creating Apple Event descriptor failed: error {0}",
+                      error));
+
+  // Deallocate the descriptor on exit.
+  auto on_exit = llvm::make_scope_exit(
+      [&]() { AEDisposeDesc(&(file_and_line_desc.descContent)); });
+
+  static std::optional<FSRef> g_app_fsref;
+  static std::string g_app_error;
+  static std::once_flag g_once_flag;
+  std::call_once(g_once_flag, [&]() {
+    if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+      LLDB_LOG(log, "Looking for external editor: {0}", external_editor);
 
-  error = ::AECreateDesc(typeUTF8Text, &file_and_line_info,
-                         sizeof(file_and_line_info),
-                         &(file_and_line_desc.descContent));
-
-  if (error != noErr) {
-    LLDB_LOGF(log, "Error creating AEDesc: %ld.\n", error);
-    return false;
+      FSRef app_fsref;
+      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
+      long app_error = ::LSFindApplicationForInfo(
+          /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
+          /*inName=*/editor_name.get(), /*outAppRef=*/&app_fsref,
+          /*outAppURL=*/NULL);
+      if (app_error == noErr) {
+        g_app_fsref = app_fsref;
+      } else {
+        g_app_error =
+            llvm::formatv("could not find external editor \"{0}\": "
+                          "LSFindApplicationForInfo returned error {1}",
+                          external_editor, app_error)
+                .str();
       }
+    }
+  });
 
-  file_and_line_desc.descKey = keyAEPosition;
-
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+  if (!g_app_error.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(), g_app_error);
 
+  // Build app launch parameters.
   LSApplicationParameters app_params;
   ::memset(&app_params, 0, sizeof(app_params));
   app_params.flags =
       kLSLaunchDefaults | kLSLaunchDontAddToRecents | kLSLaunchDontSwitch;
-
-  char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR");
-
-  if (external_editor) {
-    LLDB_LOGF(log, "Looking for external editor \"%s\".\n", external_editor);
-
-    if (g_app_name.empty() ||
-        strcmp(g_app_name.c_str(), external_editor) != 0) {
-      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-      error = ::LSFindApplicationForInfo(kLSUnknownCreator, NULL,
-                                         editor_name.get(), &g_app_fsref, NULL);
-
-      // If we found the app, then store away the name so we don't have to
-      // re-look it up.
-      if (error != noErr) {
-        LLDB_LOGF(log,
-                  "Could not find External Editor application, error: %ld.\n",
-                  error);
-        return false;
-      }
-    }
-    app_params.application = &g_app_fsref;
-  }
+  if (g_app_fsref)
+    app_params.application = &(g_app_fsref.value());
 
   ProcessSerialNumber psn;
-  CFCReleaser<CFArrayRef> file_array(
-      CFArrayCreate(NULL, (const void **)file_URL.ptr_address(false), 1, NULL));
-  error = ::LSOpenURLsWithRole(file_array.get(), kLSRolesAll,
-                               &file_and_line_desc, &app_params, &psn, 1);
-
-  AEDisposeDesc(&(file_and_line_desc.descContent));
-
-  if (error != noErr) {
-    LLDB_LOGF(log, "LSOpenURLsWithRole failed, error: %ld.\n", error);
+  std::array<CFURLRef, 1> file_array = {file_URL.get()};
+  CFCReleaser<CFArrayRef> cf_array(
+      CFArrayCreate(/*allocator=*/NULL, /*values=*/(const void **)&file_array,
+                    /*numValues*/ 1, /*callBacks=*/NULL));
+  error = ::LSOpenURLsWithRole(
+      /*inURLs=*/cf_array.get(), /*inRole=*/kLSRolesEditor,
+      /*inAEParam=*/&file_and_line_desc,
+      /*inAppParams=*/&app_params, /*outPSNs=*/&psn, /*inMaxPSNCount=*/1);
+
+  if (error != noErr)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        llvm::formatv("LSOpenURLsWithRole failed: error {0}", error));
 
-    return false;
-  }
-
-  return true;
+  return llvm::Error::success();
 #endif // TARGET_OS_OSX
 }
 
 bool Host::IsInteractiveGraphicSession() {
 #if !TARGET_OS_OSX
   return false;
 #else
   auditinfo_addr_t info;

lldb/source/Interpreter/CommandInterpreter.cpp

bool CommandInterpreter::SaveTranscript(
 
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   result.AppendMessageWithFormat("Session's transcripts saved to %s\n",
                                  output_file->c_str());
 
   if (GetOpenTranscriptInEditor() && Host::IsInteractiveGraphicSession()) {
     const FileSpec file_spec;
     error = file->GetFileSpec(const_cast<FileSpec &>(file_spec));
-    if (error.Success())
-      Host::OpenFileInExternalEditor(file_spec, 1);
+    if (error.Success()) {
+      if (llvm::Error e = Host::OpenFileInExternalEditor(file_spec, 1))
+        result.AppendError(llvm::toString(std::move(e)));
+    }
   }
 
   return true;
 }
 
 bool CommandInterpreter::IsInteractive() {
   return (GetIOHandler() ? GetIOHandler()->GetIsInteractive() : false);
 }

lldb/source/Target/Thread.cpp

bool Thread::SetSelectedFrameByIndexNoisily(uint32_t frame_idx,
   if (success) {
     StackFrameSP frame_sp = GetSelectedFrame(DoNoSelectMostRelevantFrame);
     if (frame_sp) {
       bool already_shown = false;
       SymbolContext frame_sc(
           frame_sp->GetSymbolContext(eSymbolContextLineEntry));
       if (GetProcess()->GetTarget().GetDebugger().GetUseExternalEditor() &&
           frame_sc.line_entry.file && frame_sc.line_entry.line != 0) {
-        already_shown = Host::OpenFileInExternalEditor(
-            frame_sc.line_entry.file, frame_sc.line_entry.line);
+        if (llvm::Error e = Host::OpenFileInExternalEditor(
+                frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+          LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
+                         "OpenFileInExternalEditor failed: {0}");
+        } else {
+          already_shown = true;
+        }
       }
 
       bool show_frame_info = true;
       bool show_source = !already_shown;
       FrameSelectedCallback(frame_sp.get());
       return frame_sp->GetStatus(output_stream, show_frame_info, show_source);
     }
     return false;
if (!only_stacks) {
     }
     strm.Printf("%c ", is_selected ? '*' : ' ');
     if (target && target->GetDebugger().GetUseExternalEditor()) {
       StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
       if (frame_sp) {
         SymbolContext frame_sc(
             frame_sp->GetSymbolContext(eSymbolContextLineEntry));
         if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
-          Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
-                                         frame_sc.line_entry.line);
+          if (llvm::Error e = Host::OpenFileInExternalEditor(
+                  frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+            LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
+                           "OpenFileInExternalEditor failed: {0}");
+          }
         }
       }
     }
 
     DumpUsingSettingsFormat(strm, start_frame, stop_format);
   }
 
   size_t num_frames_shown = 0;