[Lldb-commits] [PATCH] D149397: Host: generalise `GetXcodeSDKPath`

[Douglas Yung via Phabricator via lldb-commits]

dyung added a comment.

I’m also seeing this fail on 2 buildbots:

https://lab.llvm.org/buildbot/#/builders/217/builds/20568
https://lab.llvm.org/buildbot/#/builders/243/builds/5576


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149397/new/

https://reviews.llvm.org/D149397

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, mib, jingham.
Herald added a project: All.
JDevlieghere requested review of this revision.

Add a setting (`debugger.external-editor`) to specify an external editor 
instead of having to rely on the `LLDB_EXTERNAL_EDITOR` environment variable.


https://reviews.llvm.org/D149565

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Host/Host.h
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Thread.cpp
  lldb/test/Shell/Settings/TestExternalEditor.test

Index: lldb/test/Shell/Settings/TestExternalEditor.test
===
--- /dev/null
+++ lldb/test/Shell/Settings/TestExternalEditor.test
@@ -0,0 +1,3 @@
+REQUIRES: system-darwin
+RUN: LLDB_EXTERNAL_EDITOR="foo" %lldb -o 'settings set use-external-editor true' -o 'setting set external-editor bar' -o 'session save' -b 2>&1 | FileCheck %s
+CHECK: error: Conflicting values for LLDB_EXTERNAL_EDITOR (foo) and debugger.external-editor (bar)
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -303,10 +303,12 @@
   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) {
+  const Debugger &debugger = GetProcess()->GetTarget().GetDebugger();
+  if (debugger.GetUseExternalEditor() && frame_sc.line_entry.file &&
+  frame_sc.line_entry.line != 0) {
 if (llvm::Error e = Host::OpenFileInExternalEditor(
-frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+debugger.GetExternalEditor(), frame_sc.line_entry.file,
+frame_sc.line_entry.line)) {
   LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
  "OpenFileInExternalEditor failed: {0}");
 } else {
@@ -1731,6 +1733,7 @@
 frame_sp->GetSymbolContext(eSymbolContextLineEntry));
 if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
   if (llvm::Error e = Host::OpenFileInExternalEditor(
+  target->GetDebugger().GetExternalEditor(),
   frame_sc.line_entry.file, frame_sc.line_entry.line)) {
 LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
"OpenFileInExternalEditor failed: {0}");
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3272,7 +3272,8 @@
 const FileSpec file_spec;
 error = file->GetFileSpec(const_cast(file_spec));
 if (error.Success()) {
-  if (llvm::Error e = Host::OpenFileInExternalEditor(file_spec, 1))
+  if (llvm::Error e = Host::OpenFileInExternalEditor(
+  m_debugger.GetExternalEditor(), file_spec, 1))
 result.AppendError(llvm::toString(std::move(e)));
 }
   }
Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -325,7 +325,8 @@
 
 #endif // TARGET_OS_OSX
 
-llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(llvm::StringRef editor,
+   const FileSpec &file_spec,
uint32_t line_no) {
 #if !TARGET_OS_OSX
   return llvm::errorCodeToError(
@@ -391,41 +392,45 @@
   auto on_exit = llvm::make_scope_exit(
   [&]() { AEDisposeDesc(&(file_and_line_desc.descContent)); });
 
-  static std::optional 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);
-
-  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_

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:298
 
+  llvm::StringRef GetExternalEditor() const;
+  bool SetExternalEditor(llvm::StringRef editor);

newline



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:395-396
 
-  static std::optional 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);
-
-  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();
+  if (const char *lldb_external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+if (!editor.empty()) {
+  if (editor != llvm::StringRef(lldb_external_editor)) {

Should we even check for the env variable is the user set the editor setting ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149565/new/

https://reviews.llvm.org/D149565

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:298
 
+  llvm::StringRef GetExternalEditor() const;
+  bool SetExternalEditor(llvm::StringRef editor);

mib wrote:
> newline
This was intentional but I'll back it out of this patch and clean up everything 
in a separate commit. 



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:395-396
 
-  static std::optional 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);
-
-  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();
+  if (const char *lldb_external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+if (!editor.empty()) {
+  if (editor != llvm::StringRef(lldb_external_editor)) {

mib wrote:
> Should we even check for the env variable is the user set the editor setting ?
I asked that question in D149472  and this was Jim's suggestion: 
https://reviews.llvm.org/D149472#inline-1443754


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149565/new/

https://reviews.llvm.org/D149565

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149567: [lldb] Group debugger properties (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: mib, bulbazord.
Herald added a project: All.
JDevlieghere requested review of this revision.

Group debugger properties in Doxygen groups.


https://reviews.llvm.org/D149567

Files:
  lldb/include/lldb/Core/Debugger.h

Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -249,112 +249,130 @@
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  // Properties Functions
-  enum StopDisassemblyType {
-eStopDisassemblyTypeNever = 0,
-eStopDisassemblyTypeNoDebugInfo,
-eStopDisassemblyTypeNoSource,
-eStopDisassemblyTypeAlways
-  };
-
   Status SetPropertyValue(const ExecutionContext *exe_ctx,
   VarSetOperationType op, llvm::StringRef property_path,
   llvm::StringRef value) override;
 
-  bool GetAutoConfirm() const;
-
+  /// Format properties.
+  /// @{
   const FormatEntity::Entry *GetDisassemblyFormat() const;
-
   const FormatEntity::Entry *GetFrameFormat() const;
-
   const FormatEntity::Entry *GetFrameFormatUnique() const;
-
-  uint32_t GetStopDisassemblyMaxSize() const;
-
   const FormatEntity::Entry *GetThreadFormat() const;
-
   const FormatEntity::Entry *GetThreadStopFormat() const;
+  /// @}
 
+  /// Scripting language properties.
+  /// @{
   lldb::ScriptLanguage GetScriptLanguage() const;
-
   bool SetScriptLanguage(lldb::ScriptLanguage script_lang);
+  /// @}
 
+  /// REPL language properties.
+  /// @{
   lldb::LanguageType GetREPLLanguage() const;
-
   bool SetREPLLanguage(lldb::LanguageType repl_lang);
+  /// @}
 
+  /// Terminal width properties.
+  /// @{
   uint32_t GetTerminalWidth() const;
-
   bool SetTerminalWidth(uint32_t term_width);
+  /// @}
 
+  /// Prompt properties.
+  /// @{
   llvm::StringRef GetPrompt() const;
-
   void SetPrompt(llvm::StringRef p);
   void SetPrompt(const char *) = delete;
+  /// @}
 
+  /// External editor properties.
+  /// @{
   bool GetUseExternalEditor() const;
-
   bool SetUseExternalEditor(bool use_external_editor_p);
+  /// @}
 
+  /// Color properties.
+  /// @{
   bool GetUseColor() const;
-
   bool SetUseColor(bool use_color);
+  /// @}
 
+  /// Progress properties.
+  /// @{
   bool GetShowProgress() const;
-
   bool SetShowProgress(bool show_progress);
 
   llvm::StringRef GetShowProgressAnsiPrefix() const;
-
   llvm::StringRef GetShowProgressAnsiSuffix() const;
+  /// @}
 
+  /// Autosuggestion properties.
+  /// @{
   bool GetUseAutosuggestion() const;
-
   llvm::StringRef GetAutosuggestionAnsiPrefix() const;
-
   llvm::StringRef GetAutosuggestionAnsiSuffix() const;
+  /// @}
 
+  /// Source properties.
+  /// @{
   bool GetUseSourceCache() const;
-
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetHighlightSource() const;
+  /// @}
 
+  /// Stop properties.
+  /// @{
   lldb::StopShowColumn GetStopShowColumn() const;
-
   llvm::StringRef GetStopShowColumnAnsiPrefix() const;
-
   llvm::StringRef GetStopShowColumnAnsiSuffix() const;
 
   uint32_t GetStopSourceLineCount(bool before) const;
+  uint32_t GetDisassemblyLineCount() const;
 
-  StopDisassemblyType GetStopDisassemblyDisplay() const;
+  enum StopDisassemblyType {
+eStopDisassemblyTypeNever = 0,
+eStopDisassemblyTypeNoDebugInfo,
+eStopDisassemblyTypeNoSource,
+eStopDisassemblyTypeAlways
+  };
 
-  uint32_t GetDisassemblyLineCount() const;
+  StopDisassemblyType GetStopDisassemblyDisplay() const;
+  uint32_t GetStopDisassemblyMaxSize() const;
 
   llvm::StringRef GetStopShowLineMarkerAnsiPrefix() const;
-
   llvm::StringRef GetStopShowLineMarkerAnsiSuffix() const;
+  /// @}
 
-  bool GetAutoOneLineSummaries() const;
-
+  /// Auto indentation properties.
+  /// @{
   bool GetAutoIndent() const;
-
   bool SetAutoIndent(bool b);
+  /// @}
 
+  /// Expression properties.
+  /// @{
   bool GetPrintDecls() const;
-
   bool SetPrintDecls(bool b);
 
-  uint32_t GetTabSize() const;
+  bool GetNotifyVoid() const;
 
+  lldb::DWIMPrintVerbosity GetDWIMPrintVerbosity() const;
+  /// @}
+
+  /// Tab size properties.
+  /// @{
+  uint32_t GetTabSize() const;
   bool SetTabSize(uint32_t tab_size);
+  /// @}
 
-  lldb::DWIMPrintVerbosity GetDWIMPrintVerbosity() const;
+  bool GetAutoConfirm() const;
 
   bool GetEscapeNonPrintables() const;
 
-  bool GetNotifyVoid() const;
+  bool GetAutoOneLineSummaries() const;
 
   ConstString GetInstanceName() { return m_instance_name; }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149567: [lldb] Group debugger properties (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

nit: Sometimes we have newlines between methods and sometimes we don't ... It 
would be nice to be consistent, otherwise LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149567/new/

https://reviews.llvm.org/D149567

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149565: [lldb] Add debugger.external-editor setting

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149565/new/

https://reviews.llvm.org/D149565

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149567: [lldb] Group debugger properties (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D149567#4309195 , @mib wrote:

> nit: Sometimes we have newlines between methods and sometimes we don't ... It 
> would be nice to be consistent, otherwise LGTM

Methods relating to the same property are grouped together (i.e. no newline) 
and methods relating to different properties are separated by a newline.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149567/new/

https://reviews.llvm.org/D149567

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149567: [lldb] Group debugger properties (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 518385.
JDevlieghere edited the summary of this revision.
JDevlieghere added a comment.

Use one big Doxygen group for all the properties to make this less noisy.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149567/new/

https://reviews.llvm.org/D149567

Files:
  lldb/include/lldb/Core/Debugger.h

Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -249,112 +249,91 @@
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  // Properties Functions
-  enum StopDisassemblyType {
-eStopDisassemblyTypeNever = 0,
-eStopDisassemblyTypeNoDebugInfo,
-eStopDisassemblyTypeNoSource,
-eStopDisassemblyTypeAlways
-  };
-
+  /// Debugger properties.
+  /// @{
   Status SetPropertyValue(const ExecutionContext *exe_ctx,
   VarSetOperationType op, llvm::StringRef property_path,
   llvm::StringRef value) override;
 
-  bool GetAutoConfirm() const;
-
   const FormatEntity::Entry *GetDisassemblyFormat() const;
-
   const FormatEntity::Entry *GetFrameFormat() const;
-
   const FormatEntity::Entry *GetFrameFormatUnique() const;
-
-  uint32_t GetStopDisassemblyMaxSize() const;
-
   const FormatEntity::Entry *GetThreadFormat() const;
-
   const FormatEntity::Entry *GetThreadStopFormat() const;
 
   lldb::ScriptLanguage GetScriptLanguage() const;
-
   bool SetScriptLanguage(lldb::ScriptLanguage script_lang);
 
   lldb::LanguageType GetREPLLanguage() const;
-
   bool SetREPLLanguage(lldb::LanguageType repl_lang);
 
   uint32_t GetTerminalWidth() const;
-
   bool SetTerminalWidth(uint32_t term_width);
 
   llvm::StringRef GetPrompt() const;
-
   void SetPrompt(llvm::StringRef p);
   void SetPrompt(const char *) = delete;
 
   bool GetUseExternalEditor() const;
-
   bool SetUseExternalEditor(bool use_external_editor_p);
 
   bool GetUseColor() const;
-
   bool SetUseColor(bool use_color);
 
   bool GetShowProgress() const;
-
   bool SetShowProgress(bool show_progress);
 
   llvm::StringRef GetShowProgressAnsiPrefix() const;
-
   llvm::StringRef GetShowProgressAnsiSuffix() const;
 
   bool GetUseAutosuggestion() const;
-
   llvm::StringRef GetAutosuggestionAnsiPrefix() const;
-
   llvm::StringRef GetAutosuggestionAnsiSuffix() const;
 
   bool GetUseSourceCache() const;
-
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetHighlightSource() const;
 
   lldb::StopShowColumn GetStopShowColumn() const;
-
   llvm::StringRef GetStopShowColumnAnsiPrefix() const;
-
   llvm::StringRef GetStopShowColumnAnsiSuffix() const;
 
   uint32_t GetStopSourceLineCount(bool before) const;
+  uint32_t GetDisassemblyLineCount() const;
 
-  StopDisassemblyType GetStopDisassemblyDisplay() const;
+  enum StopDisassemblyType {
+eStopDisassemblyTypeNever = 0,
+eStopDisassemblyTypeNoDebugInfo,
+eStopDisassemblyTypeNoSource,
+eStopDisassemblyTypeAlways
+  };
 
-  uint32_t GetDisassemblyLineCount() const;
+  StopDisassemblyType GetStopDisassemblyDisplay() const;
+  uint32_t GetStopDisassemblyMaxSize() const;
 
   llvm::StringRef GetStopShowLineMarkerAnsiPrefix() const;
-
   llvm::StringRef GetStopShowLineMarkerAnsiSuffix() const;
 
-  bool GetAutoOneLineSummaries() const;
-
   bool GetAutoIndent() const;
-
   bool SetAutoIndent(bool b);
 
   bool GetPrintDecls() const;
-
   bool SetPrintDecls(bool b);
 
-  uint32_t GetTabSize() const;
+  bool GetNotifyVoid() const;
+
+  lldb::DWIMPrintVerbosity GetDWIMPrintVerbosity() const;
 
+  uint32_t GetTabSize() const;
   bool SetTabSize(uint32_t tab_size);
 
-  lldb::DWIMPrintVerbosity GetDWIMPrintVerbosity() const;
+  bool GetAutoConfirm() const;
 
   bool GetEscapeNonPrintables() const;
 
-  bool GetNotifyVoid() const;
+  bool GetAutoOneLineSummaries() const;
+  /// @}
 
   ConstString GetInstanceName() { return m_instance_name; }
 
@@ -371,19 +350,19 @@
   bool IsHandlingEvents() const { return m_event_handler_thread.IsJoinable(); }
 
   Status RunREPL(lldb::LanguageType language, const char *repl_options);
-  
+
   /// Interruption in LLDB:
-  /// 
+  ///
   /// This is a voluntary interruption mechanism, not preemptive.  Parts of lldb
-  /// that do work that can be safely interrupted call 
+  /// that do work that can be safely interrupted call
   /// Debugger::InterruptRequested and if that returns true, they should return
   /// at a safe point, shortcutting the rest of the work they were to do.
-  ///  
-  /// lldb clients can both offer a CommandInterpreter (through 
+  ///
+  /// lldb clients can both offer a CommandInterpreter (through
   /// RunCommandInterpreter) and use the SB API's for their own purposes, so it
   /// is convenient to separate "interrupting the CommandInterpreter execution"
-  /// and interrupting the work it is doing with the SB API's.  So there are two 
-  /// ways to cause an interrupt: