[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere closed this revision.
JDevlieghere added a comment.

13dbc16b4d82 



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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere reopened this revision.
JDevlieghere added a comment.

That's my bad: I put the wrong differential URL in my commit 
(13dbc16b4d82b9adc98c0919873b2b59ccc46afd 
)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5679379cc7df: Refactor and generalize AArch64 watchpoint 
support in debugserver (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

Files:
  
lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile
  
lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py
  lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/main.c
  lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
  lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h

Index: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
===
--- lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
+++ lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
@@ -13,6 +13,7 @@
 
 #include 
 #include 
+#include 
 
 #if defined(ARM_THREAD_STATE64_COUNT)
 
@@ -35,6 +36,13 @@
 memset(&m_dbg_save, 0, sizeof(m_dbg_save));
   }
 
+  struct WatchpointSpec {
+nub_addr_t aligned_start;
+nub_addr_t requested_start;
+nub_size_t aligned_size;
+nub_size_t requested_size;
+  };
+
   virtual ~DNBArchMachARM64() {}
 
   static void Initialize();
@@ -71,8 +79,14 @@
 bool also_set_on_task) override;
   bool DisableHardwareBreakpoint(uint32_t hw_break_index,
  bool also_set_on_task) override;
+  std::vector
+  AlignRequestedWatchpoint(nub_addr_t requested_addr,
+   nub_size_t requested_size);
   uint32_t EnableHardwareWatchpoint(nub_addr_t addr, nub_size_t size, bool read,
 bool write, bool also_set_on_task) override;
+  uint32_t SetBASWatchpoint(WatchpointSpec wp, bool read, bool write,
+bool also_set_on_task);
+  uint32_t SetMASKWatchpoint(WatchpointSpec wp);
   bool DisableHardwareWatchpoint(uint32_t hw_break_index,
  bool also_set_on_task) override;
   bool DisableHardwareWatchpoint_helper(uint32_t hw_break_index,
Index: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
===
--- lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
+++ lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
@@ -830,6 +830,69 @@
   return INVALID_NUB_HW_INDEX;
 }
 
+std::vector
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t requested_addr,
+   nub_size_t requested_size) {
+
+  // Can't watch zero bytes
+  if (requested_size == 0)
+return {};
+
+  // Smallest size we can watch on AArch64 is 8 bytes
+  constexpr nub_size_t min_watchpoint_alignment = 8;
+  nub_size_t aligned_size = std::max(requested_size, min_watchpoint_alignment);
+
+  // AArch64 addresses are 8 bytes.
+  constexpr int addr_byte_size = 8;
+  constexpr int addr_bit_size = addr_byte_size * 8;
+
+  /// Round up \a requested_size to the next power-of-2 size, at least 8
+  /// bytes
+  /// requested_size == 3  -> aligned_size == 8
+  /// requested_size == 13 -> aligned_size == 16
+  /// requested_size == 16 -> aligned_size == 16
+  /// Could be `std::bit_ceil(aligned_size)` when we build with C++20?
+  aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+
+  nub_addr_t aligned_start = requested_addr & ~(aligned_size - 1);
+  // Does this power-of-2 memory range, aligned to power-of-2, completely
+  // encompass the requested watch region.
+  if (aligned_start + aligned_size >= requested_addr + requested_size) {
+WatchpointSpec wp;
+wp.aligned_start = aligned_start;
+wp.requested_start = requested_addr;
+wp.aligned_size = aligned_size;
+wp.requested_size = requested_size;
+return {{wp}};
+  }
+
+  // We need to split this into two watchpoints, split on the aligned_size
+  // boundary and re-evaluate the alignment of each half.
+  //
+  // requested_addr 48 requested_size 20 -> aligned_size 32
+  //  aligned_start 32
+  //  split_addr 64
+  //  first_requested_addr 48
+  //  first_requested_size 16
+  //  second_requested_addr 64
+  //  second_requested_size 4
+  nub_addr_t split_addr = aligned_start + aligned_size;
+
+  nub_addr_t first_requested_addr = requested_addr;
+  nub_size_t first_requested_size = split_addr - requested_addr;
+  nub_addr_t second_requested_addr = split_addr;
+  nub_size_t second_requested_size = requested_size - first_requested_size;
+
+  std::vector first_wp =
+  AlignRequestedWatchpoint(first_requested_addr, first_requested_size);
+  std::vector 

[Lldb-commits] [lldb] 5679379 - Refactor and generalize AArch64 watchpoint support in debugserver

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-04-28T18:24:38-07:00
New Revision: 5679379cc7df198a73c137e691346088338f

URL: 
https://github.com/llvm/llvm-project/commit/5679379cc7df198a73c137e691346088338f
DIFF: 
https://github.com/llvm/llvm-project/commit/5679379cc7df198a73c137e691346088338f.diff

LOG: Refactor and generalize AArch64 watchpoint support in debugserver

Refactor the debugserver watchpiont support in anticipating of
adding support for AArch64 MASK hardware watchpoints to watch
larger regions of memory.  debugserver already had support for
handling a request to watch an unaligned region of memory up
to 8 bytes using Byte Address Select watchpoints - it would split
an unaligned watch request into two aligned doublewords that
could be watched with two hardware watchpoints using the BAS
specification.

This patch generalizes that code for properly aligning, and
possibly splitting, a watchpoint request into two hardware watchpoints
to handle any size request.  And separates out the specifics
about BAS watchpoints into its own method, so a sibling method
for MASK watchpoints can be dropped in next.

Differential Revision: https://reviews.llvm.org/D149040
rdar://108233371

Added: 

lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile

lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py

lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/main.c

Modified: 
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h

Removed: 




diff  --git 
a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile
 
b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile
new file mode 100644
index 0..10495940055b6
--- /dev/null
+++ 
b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules

diff  --git 
a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py
 
b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py
new file mode 100644
index 0..62ed1280d51d1
--- /dev/null
+++ 
b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py
@@ -0,0 +1,61 @@
+"""
+Watch 4 bytes which spawn two doubleword aligned regions.
+On a target that supports 8 byte watchpoints, this will
+need to be implemented with a hardware watchpoint on both
+doublewords.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class UnalignedWatchpointTestCase(TestBase):
+
+def hit_watchpoint_and_continue(self, process, iter_str):
+process.Continue()
+self.assertEqual(process.GetState(), lldb.eStateStopped,
+ iter_str)
+thread = process.GetSelectedThread()
+self.assertEqual(thread.GetStopReason(), lldb.eStopReasonWatchpoint, 
iter_str)
+self.assertEqual(thread.GetStopReasonDataCount(), 1, iter_str)
+wp_num = thread.GetStopReasonDataAtIndex(0)
+self.assertEqual(wp_num, 1, iter_str)
+
+NO_DEBUG_INFO_TESTCASE = True
+# debugserver on AArch64 has this feature.
+@skipIf(archs=no_match(['x86_64', 'arm64', 'arm64e', 'aarch64']))
+@skipUnlessDarwin
+# debugserver only started returning an exception address within
+# a range lldb expects in https://reviews.llvm.org/D147820 2023-04-12.
+# older debugservers will return the base address of the doubleword
+# which lldb doesn't understand, and will stop executing without a
+# proper stop reason.
+@skipIfOutOfTreeDebugserver
+
+def test_unaligned_watchpoint(self):
+"""Test a watchpoint that is handled by two hardware watchpoint 
registers."""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+(target, process, thread, bkpt) = 
lldbutil.run_to_source_breakpoint(self,
+ "break here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+
+a_bytebuf_6 = frame.GetValueForVariablePath("a.bytebuf[6]")
+a_bytebuf_6_addr = a_bytebuf_6.GetAddress().GetLoadAddress(target)
+err = lldb.SBError()
+wp = target.WatchAddress(a_bytebuf_6_addr, 4, False, True, err)
+self.assertTrue(err.Success())
+self.assertTrue(wp.IsEnabled())
+self.assertEqual(wp.GetWatchSize(), 4)
+self.assertGreater(wp.GetWatchAddress() % 8, 4, "watched region spans 
two doublewords")
+
+# We will hit our watchpoint 6 times during the execution
+# of the inferior.  If the remote stub does not actually split
+# the watched reg

[Lldb-commits] [lldb] 13dbc16 - [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-04-28T18:12:41-07:00
New Revision: 13dbc16b4d82b9adc98c0919873b2b59ccc46afd

URL: 
https://github.com/llvm/llvm-project/commit/13dbc16b4d82b9adc98c0919873b2b59ccc46afd
DIFF: 
https://github.com/llvm/llvm-project/commit/13dbc16b4d82b9adc98c0919873b2b59ccc46afd.diff

LOG: [lldb] Refactor host::OpenFileInExternalEditor

This patch refactors the macOS implementation of
OpenFileInExternalEditor. It fixes an AppleEvent memory leak, the
caching of LLDB_EXTERNAL_EDITOR and speculatively fixes a crash when
CFURL is NULL (rdar://108633464). The new code also improves error
handling, readability and documents calls to the CoreFoundation Launch
Services APIs.

A bunch of the Launch Services APIs have been deprecated
(LSFindApplicationForInfo, LSOpenURLsWithRole). The preferred API is
LSOpenCFURLRef but it doesn't specifying the "location" Apple Event
which is used to highlight the current line and switching over would
regress the existing behavior.

rdar://108633464

Differential revision: https://reviews.llvm.org/D149482

Added: 


Modified: 
lldb/include/lldb/Host/Host.h
lldb/source/Host/common/Host.cpp
lldb/source/Host/macosx/objcxx/Host.mm
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Target/Thread.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h
index 4fc2bd128b025..3fdf59dfb2324 100644
--- a/lldb/include/lldb/Host/Host.h
+++ b/lldb/include/lldb/Host/Host.h
@@ -236,8 +236,8 @@ class Host {
 bool run_in_shell = true,
 bool hide_stderr = false);
 
-  static bool OpenFileInExternalEditor(const FileSpec &file_spec,
-   uint32_t line_no);
+  static llvm::Error OpenFileInExternalEditor(const FileSpec &file_spec,
+  uint32_t line_no);
 
   /// Check if we're running in an interactive graphical session.
   ///

diff  --git a/lldb/source/Host/common/Host.cpp 
b/lldb/source/Host/common/Host.cpp
index 33b550008b746..c8ebb6f84c004 100644
--- a/lldb/source/Host/common/Host.cpp
+++ b/lldb/source/Host/common/Host.cpp
@@ -546,9 +546,10 @@ void Host::Kill(lldb::pid_t pid, int signo) { ::kill(pid, 
signo); }
 #endif
 
 #if !defined(__APPLE__)
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
-uint32_t line_no) {
-  return false;
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+   uint32_t line_no) {
+  return llvm::errorCodeToError(
+  std::error_code(ENOTSUP, std::system_category()));
 }
 
 bool Host::IsInteractiveGraphicSession() { return false; }

diff  --git a/lldb/source/Host/macosx/objcxx/Host.mm 
b/lldb/source/Host/macosx/objcxx/Host.mm
index 303a138559e93..848fa0d79f4da 100644
--- a/lldb/source/Host/macosx/objcxx/Host.mm
+++ b/lldb/source/Host/macosx/objcxx/Host.mm
@@ -325,12 +325,36 @@ repeat with the_window in (get windows)\n\
 
 #endif // TARGET_OS_OSX
 
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
-uint32_t line_no) {
+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() ? "" : 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 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;
@@ -340,18 +364,7 @@ repeat with the_window in (get windows)\n\
 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 file_URL(::CFURLCreateWithFileSystemPath(
-  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
-

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG13dbc16b4d82: [lldb] Refactor host::OpenFileInExternalEditor 
(authored by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D149482?vs=518041&id=518109#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Thread.cpp

Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -305,8 +305,13 @@
   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;
@@ -1725,8 +1730,11 @@
 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}");
+  }
 }
   }
 }
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3271,8 +3271,10 @@
   if (GetOpenTranscriptInEditor() && Host::IsInteractiveGraphicSession()) {
 const FileSpec file_spec;
 error = file->GetFileSpec(const_cast(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;
Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -325,12 +325,36 @@
 
 #endif // TARGET_OS_OSX
 
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
-uint32_t line_no) {
+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() ? "" : 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 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;
@@ -340,18 +364,7 @@
 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 file_URL(::CFURLCreateWithFileSystemPath(
-  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
-
-  LLDB_LOGF(log,
-   

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Thanks. This LGTM.




Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:856
+  // user_size == 16 -> aligned_size == 16
+  aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+

jasonmolenda wrote:
> tschuett wrote:
> > JDevlieghere wrote:
> > > Beautiful. Once we have C++20 we can use `std::bit_ceil` 
> > > (https://en.cppreference.com/w/cpp/numeric/bit_ceil)
> > Is the builtin available on all supported platforms and compilers? There 
> > are some alignment functions in MathExtras.h.
> Ah, that would be very nice and a lot clearer for readers.  I might add that 
> as a comment.
Yes: `debugserver` is only supported on macOS


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

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

[Lldb-commits] [PATCH] D149503: Remove i386 and armv7 native support from debugserver

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM 🥳


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149503

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

+1 on making the output deterministic with a sort


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 518101.
jasonmolenda marked an inline comment as done.
jasonmolenda added a comment.

Update patch to incorporate Jonas' feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

Files:
  
lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/Makefile
  
lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py
  lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/main.c
  lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
  lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h

Index: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
===
--- lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
+++ lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
@@ -13,6 +13,7 @@
 
 #include 
 #include 
+#include 
 
 #if defined(ARM_THREAD_STATE64_COUNT)
 
@@ -35,6 +36,13 @@
 memset(&m_dbg_save, 0, sizeof(m_dbg_save));
   }
 
+  struct WatchpointSpec {
+nub_addr_t aligned_start;
+nub_addr_t requested_start;
+nub_size_t aligned_size;
+nub_size_t requested_size;
+  };
+
   virtual ~DNBArchMachARM64() {}
 
   static void Initialize();
@@ -71,8 +79,14 @@
 bool also_set_on_task) override;
   bool DisableHardwareBreakpoint(uint32_t hw_break_index,
  bool also_set_on_task) override;
+  std::vector
+  AlignRequestedWatchpoint(nub_addr_t requested_addr,
+   nub_size_t requested_size);
   uint32_t EnableHardwareWatchpoint(nub_addr_t addr, nub_size_t size, bool read,
 bool write, bool also_set_on_task) override;
+  uint32_t SetBASWatchpoint(WatchpointSpec wp, bool read, bool write,
+bool also_set_on_task);
+  uint32_t SetMASKWatchpoint(WatchpointSpec wp);
   bool DisableHardwareWatchpoint(uint32_t hw_break_index,
  bool also_set_on_task) override;
   bool DisableHardwareWatchpoint_helper(uint32_t hw_break_index,
Index: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
===
--- lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
+++ lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
@@ -830,6 +830,69 @@
   return INVALID_NUB_HW_INDEX;
 }
 
+std::vector
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t requested_addr,
+   nub_size_t requested_size) {
+
+  // Can't watch zero bytes
+  if (requested_size == 0)
+return {};
+
+  // Smallest size we can watch on AArch64 is 8 bytes
+  constexpr nub_size_t min_watchpoint_alignment = 8;
+  nub_size_t aligned_size = std::max(requested_size, min_watchpoint_alignment);
+
+  // AArch64 addresses are 8 bytes.
+  constexpr int addr_byte_size = 8;
+  constexpr int addr_bit_size = addr_byte_size * 8;
+
+  /// Round up \a requested_size to the next power-of-2 size, at least 8
+  /// bytes
+  /// requested_size == 3  -> aligned_size == 8
+  /// requested_size == 13 -> aligned_size == 16
+  /// requested_size == 16 -> aligned_size == 16
+  /// Could be `std::bit_ceil(aligned_size)` when we build with C++20?
+  aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+
+  nub_addr_t aligned_start = requested_addr & ~(aligned_size - 1);
+  // Does this power-of-2 memory range, aligned to power-of-2, completely
+  // encompass the requested watch region.
+  if (aligned_start + aligned_size >= requested_addr + requested_size) {
+WatchpointSpec wp;
+wp.aligned_start = aligned_start;
+wp.requested_start = requested_addr;
+wp.aligned_size = aligned_size;
+wp.requested_size = requested_size;
+return {{wp}};
+  }
+
+  // We need to split this into two watchpoints, split on the aligned_size
+  // boundary and re-evaluate the alignment of each half.
+  //
+  // requested_addr 48 requested_size 20 -> aligned_size 32
+  //  aligned_start 32
+  //  split_addr 64
+  //  first_requested_addr 48
+  //  first_requested_size 16
+  //  second_requested_addr 64
+  //  second_requested_size 4
+  nub_addr_t split_addr = aligned_start + aligned_size;
+
+  nub_addr_t first_requested_addr = requested_addr;
+  nub_size_t first_requested_size = split_addr - requested_addr;
+  nub_addr_t second_requested_addr = split_addr;
+  nub_size_t second_requested_size = requested_size - first_requested_size;
+
+  std::vector first_wp =
+  AlignRequestedWatchpoint(first_requested_addr, first_requested_size);
+  std::vector second_wp =
+  AlignReq

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda marked 7 inline comments as done.
jasonmolenda added inline comments.



Comment at: 
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:833-835
+std::vector
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t user_addr,
+   nub_size_t user_size) {

jasonmolenda wrote:
> JDevlieghere wrote:
> > Do you ever expect this to return more than two watchpoints? Seems like 
> > this could be a `struct` that holds two optional `WatchpointSpec`s. I don't 
> > feel strongly about it, but it looks like you never iterate over the list 
> > and the way you have to check after the recursive call is a bit awkward. 
> > 
> > edit: nvm, it looks like you do actually iterate over them in 
> > `EnableHardwareWatchpoint`
> I was thinking about how this current scheme only ever splits 1 watchpoint 
> into 2, but a future design that could expand a user requested watch into a 
> larger number of hardware watchpoints would expand it further.  If a user 
> asks to watch a 32 byte object, and the target only supports 8 byte 
> watchpoints, and the object is doubleword aligned, we could watch it with 4 
> hardware watchpoints.  The older code in debugserver was written in terms of 
> "one or two" but I switched to a vector of hardware implementable watchpoints 
> that may expand as we evaluate the hardware capabilities of the target/stub.
There's some extent where this debugserver implementation is me sketching out 
the first part of the WatchpointLocation work I want to do in lldb.  I will 
likely be copying this code up in to lldb, so it's written with an eye to where 
I'm heading there, agreed it's not necessary tho.  tbh once that 
WatchpointLocation stuff exists in lldb, all of this can be pulled from 
debugserver.



Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:856
+  // user_size == 16 -> aligned_size == 16
+  aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+

tschuett wrote:
> JDevlieghere wrote:
> > Beautiful. Once we have C++20 we can use `std::bit_ceil` 
> > (https://en.cppreference.com/w/cpp/numeric/bit_ceil)
> Is the builtin available on all supported platforms and compilers? There are 
> some alignment functions in MathExtras.h.
Ah, that would be very nice and a lot clearer for readers.  I might add that as 
a comment.



Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:923
 
-  // Otherwise, can't watch more than 8 bytes per WVR/WCR pair
-  if (size > 8)
-return INVALID_NUB_HW_INDEX;
-
-  // Aarch64 watchpoints are in one of two forms: (1) 1-8 bytes, aligned to
-  // an 8 byte address, or (2) a power-of-two size region of memory; minimum
-  // 8 bytes, maximum 2GB; the starting address must be aligned to that power
-  // of two.
-  //
-  // For (1), 1-8 byte watchpoints, using the Byte Address Selector field in
-  // DBGWCR.BAS.  Any of the bytes may be watched, but if multiple bytes
-  // are watched, the bytes selected must be contiguous.  The start address
-  // watched must be doubleword (8-byte) aligned; if the start address is
-  // word (4-byte) aligned, only 4 bytes can be watched.
-  //
-  // For (2), the MASK field in DBGWCR.MASK is used.
-  //
-  // See the ARM ARM, section "Watchpoint exceptions", and more specifically,
-  // "Watchpoint data address comparisons".
-  //
-  // debugserver today only supports (1) - the Byte Address Selector 1-8 byte
-  // watchpoints that are 8-byte aligned.  To support larger watchpoints,
-  // debugserver would need to interpret the mach exception when the watched
-  // region was hit, see if the address accessed lies within the subset
-  // of the power-of-two region that lldb asked us to watch (v. ARM ARM,
-  // "Determining the memory location that caused a Watchpoint exception"),
-  // and silently resume the inferior (disable watchpoint, stepi, re-enable
-  // watchpoint) if the address lies outside the region that lldb asked us
-  // to watch.
-  //
-  // Alternatively, lldb would need to be prepared for a larger region
-  // being watched than it requested, and silently resume the inferior if
-  // the accessed address is outside the region lldb wants to watch.
-
-  nub_addr_t aligned_wp_address = addr & ~0x7;
-  uint32_t addr_dword_offset = addr & 0x7;
-
-  // Do we need to split up this logical watchpoint into two hardware 
watchpoint
-  // registers?
-  // e.g. a watchpoint of length 4 on address 6.  We need do this with
-  //   one watchpoint on address 0 with bytes 6 & 7 being monitored
-  //   one watchpoint on address 8 with bytes 0, 1, 2, 3 being monitored
-
-  if (addr_dword_offset + size > 8) {
-DNBLogThreadedIf(LOG_WATCHPOINTS, "DNBArchMachARM64::"
-  "EnableHardwareWatchpoint(addr = "
-  "0x%8.8llx, size = %zu) needs two "
- 

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149262#4306884 , @kuilpd wrote:

> In D149262#4306820 , @bulbazord 
> wrote:
>
>> Sorry I should have brought this up earlier but I've noticed you don't have 
>> any tests with this change. Is it possible you could add something there? 
>> Something to make sure that these settings work as expected.
>
> Should I do it also without a target?

It's probably enough to just add a test with a target, the settings shouldn't 
mess with the non-target case I think.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd added a comment.

In D149262#4306820 , @bulbazord wrote:

> Sorry I should have brought this up earlier but I've noticed you don't have 
> any tests with this change. Is it possible you could add something there? 
> Something to make sure that these settings work as expected.

Should I do it also without a target?

> Sorry for the churn btw, I really appreciate your patience here.

No worries, these are all valid points you raised :)




Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

bulbazord wrote:
> kuilpd wrote:
> > bulbazord wrote:
> > > kuilpd wrote:
> > > > bulbazord wrote:
> > > > > A few things:
> > > > > - I don't think you need to re-extract process from `exe_ctx`, the 
> > > > > variable `process` at the beginning of this function should have a 
> > > > > valid Process pointer in it already (see: `LockAndCheckContext` at 
> > > > > the beginning of this function). We already assume `target` is valid 
> > > > > and use it in the same way.
> > > > > - If we can't get an ABI from the Process, do we want to assume a 
> > > > > stack frame size of `512 * 1024`? Maybe there is a use case I'm 
> > > > > missing, but if we don't have an ABI I'm not convinced that 
> > > > > `PrepareToExecuteJITExpression` should succeed. LLDB will need some 
> > > > > kind of ABI information to correctly set up the register context to 
> > > > > call the JITed code anyway.
> > > > - I tried that, but a lot of tests fail on `GetABI()` after that, so 
> > > > had to re-extract it. Not sure why.
> > > > - `512 * 1024` is what was hardcoded there by default. It makes sense 
> > > > that ABI has to exist, but leaving no default value in case if it's not 
> > > > retreived is weird as well. Or should we return an error in that case?
> > > > 
> > > How do the tests fail? If the process is wrong that sounds like a pretty 
> > > bad bug :(
> > > 
> > > I would think that we could return `false` with some logging or an error 
> > > would be appropriate if we don't have an ABI. I may be misunderstanding 
> > > something but I would think that the tests should still pass when we 
> > > `return false` there... I hope.
> > Tried returning false, even more tests failed.
> > Did some digging, turns out expression can work without any target, right 
> > after starting LLDB.
> > So tests like [[ 
> > https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/expression/calculator_mode/TestCalculatorMode.py
> >  | this one  ]] fail because there is no target or process.
> Ah, right, I forgot! A given expression, if simple enough, might be compiled 
> to LLVM IR and then interpreted instead of being compiled to machine code and 
> run in the inferior... The fact that a stack frame size is even relevant in 
> that case is strange but there's not much we can do about it without further 
> refactors. Does something like this work then? If not, this should be fine.
> 
> ```
> if (stack_frame_size == 0) {
>   ABISP abi_sp;
>   if (process && (abi_sp = process->GetABI()))
> stack_frame_size = abi_sp->GetStackFrameSize();
>   else
> stack_frame_size = 512 * 1024;
> }
> ```
This worked, thanks!


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Core/Disassembler.cpp:846
+// Note: The reason we are making the data_type a uint64_t when value 
is
+// uint32_t is that there is no eTypeUInt32 enum value.
+if (llvm::StringRef(value) == "uint32_t")

jingham wrote:
> That's kind of answering a question with a question:  Why isn't there an 
> eTypeUInt32?
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.



Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:39
 
-for (pos = m_values.begin(); pos != end; ++pos) {
+for (auto pos = m_values.begin(), end = m_values.end(); pos != end; ++pos) 
{
   OptionValue *option_value = pos->second.get();

jingham wrote:
> Does the llvm::StringMap dingus not support `for (auto value : m_values) {` ?
Oh, it probably does. Didn't even think about it, good catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Sorry I should have brought this up earlier but I've noticed you don't have any 
tests with this change. Is it possible you could add something there? Something 
to make sure that these settings work as expected.

Sorry for the churn btw, I really appreciate your patience here.




Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

kuilpd wrote:
> bulbazord wrote:
> > kuilpd wrote:
> > > bulbazord wrote:
> > > > A few things:
> > > > - I don't think you need to re-extract process from `exe_ctx`, the 
> > > > variable `process` at the beginning of this function should have a 
> > > > valid Process pointer in it already (see: `LockAndCheckContext` at the 
> > > > beginning of this function). We already assume `target` is valid and 
> > > > use it in the same way.
> > > > - If we can't get an ABI from the Process, do we want to assume a stack 
> > > > frame size of `512 * 1024`? Maybe there is a use case I'm missing, but 
> > > > if we don't have an ABI I'm not convinced that 
> > > > `PrepareToExecuteJITExpression` should succeed. LLDB will need some 
> > > > kind of ABI information to correctly set up the register context to 
> > > > call the JITed code anyway.
> > > - I tried that, but a lot of tests fail on `GetABI()` after that, so had 
> > > to re-extract it. Not sure why.
> > > - `512 * 1024` is what was hardcoded there by default. It makes sense 
> > > that ABI has to exist, but leaving no default value in case if it's not 
> > > retreived is weird as well. Or should we return an error in that case?
> > > 
> > How do the tests fail? If the process is wrong that sounds like a pretty 
> > bad bug :(
> > 
> > I would think that we could return `false` with some logging or an error 
> > would be appropriate if we don't have an ABI. I may be misunderstanding 
> > something but I would think that the tests should still pass when we 
> > `return false` there... I hope.
> Tried returning false, even more tests failed.
> Did some digging, turns out expression can work without any target, right 
> after starting LLDB.
> So tests like [[ 
> https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/expression/calculator_mode/TestCalculatorMode.py
>  | this one  ]] fail because there is no target or process.
Ah, right, I forgot! A given expression, if simple enough, might be compiled to 
LLVM IR and then interpreted instead of being compiled to machine code and run 
in the inferior... The fact that a stack frame size is even relevant in that 
case is strange but there's not much we can do about it without further 
refactors. Does something like this work then? If not, this should be fine.

```
if (stack_frame_size == 0) {
  ABISP abi_sp;
  if (process && (abi_sp = process->GetABI()))
stack_frame_size = abi_sp->GetStackFrameSize();
  else
stack_frame_size = 512 * 1024;
}
```


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd added inline comments.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

bulbazord wrote:
> kuilpd wrote:
> > bulbazord wrote:
> > > A few things:
> > > - I don't think you need to re-extract process from `exe_ctx`, the 
> > > variable `process` at the beginning of this function should have a valid 
> > > Process pointer in it already (see: `LockAndCheckContext` at the 
> > > beginning of this function). We already assume `target` is valid and use 
> > > it in the same way.
> > > - If we can't get an ABI from the Process, do we want to assume a stack 
> > > frame size of `512 * 1024`? Maybe there is a use case I'm missing, but if 
> > > we don't have an ABI I'm not convinced that 
> > > `PrepareToExecuteJITExpression` should succeed. LLDB will need some kind 
> > > of ABI information to correctly set up the register context to call the 
> > > JITed code anyway.
> > - I tried that, but a lot of tests fail on `GetABI()` after that, so had to 
> > re-extract it. Not sure why.
> > - `512 * 1024` is what was hardcoded there by default. It makes sense that 
> > ABI has to exist, but leaving no default value in case if it's not 
> > retreived is weird as well. Or should we return an error in that case?
> > 
> How do the tests fail? If the process is wrong that sounds like a pretty bad 
> bug :(
> 
> I would think that we could return `false` with some logging or an error 
> would be appropriate if we don't have an ABI. I may be misunderstanding 
> something but I would think that the tests should still pass when we `return 
> false` there... I hope.
Tried returning false, even more tests failed.
Did some digging, turns out expression can work without any target, right after 
starting LLDB.
So tests like [[ 
https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/expression/calculator_mode/TestCalculatorMode.py
 | this one  ]] fail because there is no target or process.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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.




Comment at: lldb/source/Core/Disassembler.cpp:846
+// Note: The reason we are making the data_type a uint64_t when value 
is
+// uint32_t is that there is no eTypeUInt32 enum value.
+if (llvm::StringRef(value) == "uint32_t")

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



Comment at: lldb/source/Interpreter/OptionValueDictionary.cpp:39
 
-for (pos = m_values.begin(); pos != end; ++pos) {
+for (auto pos = m_values.begin(), end = m_values.end(); pos != end; ++pos) 
{
   OptionValue *option_value = pos->second.get();

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149482

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

kuilpd wrote:
> bulbazord wrote:
> > A few things:
> > - I don't think you need to re-extract process from `exe_ctx`, the variable 
> > `process` at the beginning of this function should have a valid Process 
> > pointer in it already (see: `LockAndCheckContext` at the beginning of this 
> > function). We already assume `target` is valid and use it in the same way.
> > - If we can't get an ABI from the Process, do we want to assume a stack 
> > frame size of `512 * 1024`? Maybe there is a use case I'm missing, but if 
> > we don't have an ABI I'm not convinced that `PrepareToExecuteJITExpression` 
> > should succeed. LLDB will need some kind of ABI information to correctly 
> > set up the register context to call the JITed code anyway.
> - I tried that, but a lot of tests fail on `GetABI()` after that, so had to 
> re-extract it. Not sure why.
> - `512 * 1024` is what was hardcoded there by default. It makes sense that 
> ABI has to exist, but leaving no default value in case if it's not retreived 
> is weird as well. Or should we return an error in that case?
> 
How do the tests fail? If the process is wrong that sounds like a pretty bad 
bug :(

I would think that we could return `false` with some logging or an error would 
be appropriate if we don't have an ABI. I may be misunderstanding something but 
I would think that the tests should still pass when we `return false` there... 
I hope.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd added inline comments.



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:38
 
+using namespace lldb;
 using namespace lldb_private;

bulbazord wrote:
> What in this file uses something from the lldb namespace now?
ABI class



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

bulbazord wrote:
> A few things:
> - I don't think you need to re-extract process from `exe_ctx`, the variable 
> `process` at the beginning of this function should have a valid Process 
> pointer in it already (see: `LockAndCheckContext` at the beginning of this 
> function). We already assume `target` is valid and use it in the same way.
> - If we can't get an ABI from the Process, do we want to assume a stack frame 
> size of `512 * 1024`? Maybe there is a use case I'm missing, but if we don't 
> have an ABI I'm not convinced that `PrepareToExecuteJITExpression` should 
> succeed. LLDB will need some kind of ABI information to correctly set up the 
> register context to call the JITed code anyway.
- I tried that, but a lot of tests fail on `GetABI()` after that, so had to 
re-extract it. Not sure why.
- `512 * 1024` is what was hardcoded there by default. It makes sense that ABI 
has to exist, but leaving no default value in case if it's not retreived is 
weird as well. Or should we return an error in that case?



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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149482: [lldb] Change ObjectValueDictionary to use a StringMap

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, mib, jingham, jasonmolenda.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149482

Files:
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -146,12 +146,12 @@
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("A");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
 
-  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -60,7 +60,7 @@
 self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
 def test_append_target_env_vars(self):
-"""Test that 'append target.run-args' works."""
+"""Test that 'append target.env-vars' works."""
 # Append the env-vars.
 self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
 # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@
 found_env_var = True
 break
 self.assertTrue(found_env_var,
-"MY_ENV_VAR was not set in LunchInfo object")
+"MY_ENV_VAR was not set in LaunchInfo object")
 
 self.assertEqual(launch_info.GetNumArguments(), 3)
 self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
@@ -604,6 +604,7 @@
 self.expect(
 "settings show target.env-vars",
 SETTING_MSG("target.env-vars"),
+ordered=False,
 substrs=[
 'target.env-vars (dictionary of strings) =',
 'A=B',
@@ -640,7 +641,7 @@
 def test_settings_remove_single(self):
 # Set some environment variables and use 'remove' to delete them.
 self.runCmd("settings set target.env-vars a=b c=d")
-self.expect("settings show target.env-vars", substrs=["a=b", "c=d"])
+self.expect("settings show target.env-vars", ordered=False, substrs=["a=b", "c=d"])
 self.runCmd("settings remove target.env-vars a")
 self.expect("settings show target.env-vars", matching=False, substrs=["a=b"])
 self.expect("settings show target.env-vars", substrs=["c=d"])
@@ -649,7 +650,7 @@
 
 def test_settings_remove_multiple(self):
 self.runCmd("settings set target.env-vars a=b c=d e=f")
-self.expect("settings show target.env-vars", substrs=["a=b", "c=d", "e=f"])
+self.expect("settings show target.env-vars", ordered=False, substrs=["a=b", "c=d", "e=f"])
 self.runCmd("settings remove target.env-vars a e")
 self.expect("settings show target.env-vars", matching=False, substrs=["a=b", "e=f"])
 self.expect("settings show target.env-vars", substrs=["c=d"])
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -300,20 +300,18 @@
 if (!module_env_option) {
   // Step 2: Try with the file name in lowercase.

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

Mostly looks good to me, just a small thing about an implementation detail.




Comment at: lldb/source/Expression/LLVMUserExpression.cpp:38
 
+using namespace lldb;
 using namespace lldb_private;

What in this file uses something from the lldb namespace now?



Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;

A few things:
- I don't think you need to re-extract process from `exe_ctx`, the variable 
`process` at the beginning of this function should have a valid Process pointer 
in it already (see: `LockAndCheckContext` at the beginning of this function). 
We already assume `target` is valid and use it in the same way.
- If we can't get an ABI from the Process, do we want to assume a stack frame 
size of `512 * 1024`? Maybe there is a use case I'm missing, but if we don't 
have an ABI I'm not convinced that `PrepareToExecuteJITExpression` should 
succeed. LLDB will need some kind of ABI information to correctly set up the 
register context to call the JITed code anyway.


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149477: [lldb/crashlog] Fix json module loading issue

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc275fd03254: [lldb/crashlog] Fix JSON ObjectFile module 
loading issue (authored by mib).

Changed prior to commit:
  https://reviews.llvm.org/D149477?vs=518022&id=518025#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149477

Files:
  lldb/examples/python/symbolication.py


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -401,6 +401,11 @@
 with open(tf.name, 'w') as f:
 f.write(json.dumps(data, indent=4))
 self.module = target.AddModule(tf.name, None, uuid_str)
+if self.module:
+# If we were able to add the module with inlined
+# symbols, we should mark it as available so 
load_module
+# does not exit early.
+self.unavailable = False
 if not self.module and not self.unavailable:
 return 'error: unable to get module for (%s) "%s"' % (
 self.arch, self.get_resolved_path())


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -401,6 +401,11 @@
 with open(tf.name, 'w') as f:
 f.write(json.dumps(data, indent=4))
 self.module = target.AddModule(tf.name, None, uuid_str)
+if self.module:
+# If we were able to add the module with inlined
+# symbols, we should mark it as available so load_module
+# does not exit early.
+self.unavailable = False
 if not self.module and not self.unavailable:
 return 'error: unable to get module for (%s) "%s"' % (
 self.arch, self.get_resolved_path())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] dc275fd - [lldb/crashlog] Fix JSON ObjectFile module loading issue

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2023-04-28T12:51:01-07:00
New Revision: dc275fd03254d67d29cc70a5a0569acf24d2280d

URL: 
https://github.com/llvm/llvm-project/commit/dc275fd03254d67d29cc70a5a0569acf24d2280d
DIFF: 
https://github.com/llvm/llvm-project/commit/dc275fd03254d67d29cc70a5a0569acf24d2280d.diff

LOG: [lldb/crashlog] Fix JSON ObjectFile module loading issue

In 27f27d15f6c9, we added a new way to use textual (JSON) object files
and symbol files with the interactive crashlog command, using the
inlined symbols from the crash report.

However, there was a missing piece after successfully adding the textual
module to the target, we didn't mark it as available causing the module
loading to exit early.

This patch addresses that issue by marking the module as available when
added successfully to the target.

Differential Revision: https://reviews.llvm.org/D149477

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/symbolication.py

Removed: 




diff  --git a/lldb/examples/python/symbolication.py 
b/lldb/examples/python/symbolication.py
index 9784c28a44766..3a42f340ea578 100755
--- a/lldb/examples/python/symbolication.py
+++ b/lldb/examples/python/symbolication.py
@@ -401,6 +401,11 @@ def add_module(self, target):
 with open(tf.name, 'w') as f:
 f.write(json.dumps(data, indent=4))
 self.module = target.AddModule(tf.name, None, uuid_str)
+if self.module:
+# If we were able to add the module with inlined
+# symbols, we should mark it as available so 
load_module
+# does not exit early.
+self.unavailable = False
 if not self.module and not self.unavailable:
 return 'error: unable to get module for (%s) "%s"' % (
 self.arch, self.get_resolved_path())



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

[Lldb-commits] [PATCH] D149477: [lldb/crashlog] Fix json module loading issue

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149477

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

[Lldb-commits] [PATCH] D149477: [lldb/crashlog] Fix json module loading issue

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added a reviewer: JDevlieghere.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

In 27f27d15f6c9 
, we added 
a new way to use textual (JSON) object files
and symbol files with the interactive crashlog command, using the
inlined symbols from the crash report.

However, there was a missing piece after successfully adding the textual
module to the target, we didn't mark it as available causing the module
loading to exit early.

This patch addresses that issue by marking the module as available when
added successfully to the target.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149477

Files:
  lldb/examples/python/symbolication.py


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -401,6 +401,11 @@
 with open(tf.name, 'w') as f:
 f.write(json.dumps(data, indent=4))
 self.module = target.AddModule(tf.name, None, uuid_str)
+if self.module:
+# If we were able to add the module with inlined
+# symbols, we should mark it as available to 
load_module
+# does not exit early.
+self.unavailable = False
 if not self.module and not self.unavailable:
 return 'error: unable to get module for (%s) "%s"' % (
 self.arch, self.get_resolved_path())


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -401,6 +401,11 @@
 with open(tf.name, 'w') as f:
 f.write(json.dumps(data, indent=4))
 self.module = target.AddModule(tf.name, None, uuid_str)
+if self.module:
+# If we were able to add the module with inlined
+# symbols, we should mark it as available to load_module
+# does not exit early.
+self.unavailable = False
 if not self.module and not self.unavailable:
 return 'error: unable to get module for (%s) "%s"' % (
 self.arch, self.get_resolved_path())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM! Thanks for cleaning this up.




Comment at: lldb/source/Target/Thread.cpp:1762-1763
+  frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
+   "OpenFileInExternalEditor failed: {0}");
+  }

Not related to this change but I don't think I like `LLDB_LOG_ERROR`... I read 
this and I think "Oh, if logging is disabled, will this actually consume the 
error?". After all, the other logging macros do not have a side effect. It'd be 
nice if we could create a function and name it "ConsumeErrorAndLog" or 
something to be clearer that it actually consumes it.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd updated this revision to Diff 518010.
kuilpd marked 4 inline comments as done.
kuilpd added a comment.

Removed unused variable arch.


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

https://reviews.llvm.org/D149262

Files:
  lldb/include/lldb/Target/ABI.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Expression/IRMemoryMap.cpp
  lldb/source/Expression/LLVMUserExpression.cpp
  lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td

Index: lldb/source/Target/TargetProperties.td
===
--- lldb/source/Target/TargetProperties.td
+++ lldb/source/Target/TargetProperties.td
@@ -24,6 +24,15 @@
 DefaultUnsignedValue<5>,
 Desc<"The maximum amount of errors to emit while parsing an expression. "
  "A value of 0 means to always continue parsing if possible.">;
+  def ExprAllocAddress: Property<"expr-alloc-address", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Start address within the process address space of memory allocation for expression evaluation.">;
+  def ExprAllocSize: Property<"expr-alloc-size", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Amount of memory in bytes to allocate for expression evaluation.">;
+  def ExprAllocAlign: Property<"expr-alloc-align", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Alignment for each memory allocation for expression evaluation.">;
   def PreferDynamic: Property<"prefer-dynamic-value", "Enum">,
 DefaultEnumValue<"eDynamicDontRunTarget">,
 EnumValues<"OptionEnumValues(g_dynamic_value_types)">,
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4587,6 +4587,24 @@
   nullptr, idx, g_target_properties[idx].default_uint_value);
 }
 
+uint64_t TargetProperties::GetExprAllocAddress() const {
+  const uint32_t idx = ePropertyExprAllocAddress;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocSize() const {
+  const uint32_t idx = ePropertyExprAllocSize;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocAlign() const {
+  const uint32_t idx = ePropertyExprAllocAlign;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
 bool TargetProperties::GetBreakpointsConsultPlatformAvoidList() {
   const uint32_t idx = ePropertyBreakpointUseAvoidList;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
===
--- lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
+++ lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
@@ -55,6 +55,8 @@
   const lldb_private::RegisterInfo *
   GetRegisterInfoArray(uint32_t &count) override;
 
+  uint64_t GetStackFrameSize() override { return 512; }
+
   //--
   // Static Functions
   //--
Index: lldb/source/Expression/LLVMUserExpression.cpp
===
--- lldb/source/Expression/LLVMUserExpression.cpp
+++ lldb/source/Expression/LLVMUserExpression.cpp
@@ -23,6 +23,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
@@ -34,6 +35,7 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+using namespace lldb;
 using namespace lldb_private;
 
 char LLVMUserExpression::ID;
@@ -333,9 +335,17 @@
 if (m_can_interpret && m_stack_frame_bottom == LLDB_INVALID_ADDRESS) {
   Status alloc_error;
 
-  auto arch = target->GetArchitecture().GetTriple().getArch();
-  const size_t stack_frame_size =
-  arch == llvm::Triple::msp430 ? 512 : 512 * 1024;
+  size_t stack_frame_size = target->GetExprAllocSize();
+  if (stack_frame_size == 0) {
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;
+}
+  }
 
   const bool zero_memory = false;
 
Index: lldb/source/Expression/IRMemoryMap.cpp
===
--- lldb/source/Expression/IRMemoryMap.cpp
+++ lldb/source/Expression/IRMemoryMap.cpp
@@ -92,26 +92,26 @@
 ret = llvm::alig

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Anton Korobeynikov via Phabricator via lldb-commits]

asl added inline comments.



Comment at: lldb/source/Expression/IRMemoryMap.cpp:181
 size_t alloc_size = back->second.m_size;
 auto arch = target_sp->GetArchitecture().GetTriple().getArch();
+uint64_t align = target_sp->GetExprAllocAlign();

is arch unused now?


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

https://reviews.llvm.org/D149262

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 518007.
JDevlieghere marked 10 inline comments as done.
JDevlieghere added a comment.

- Return an `llvm::Erorr` and percolate errors up
- Don't fall back to the default editor if the one specified can't be found
- Limit roles to editors only


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

https://reviews.llvm.org/D149472

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Thread.cpp

Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -305,8 +305,13 @@
   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;
@@ -1752,8 +1757,11 @@
 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}");
+  }
 }
   }
 }
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3241,8 +3241,10 @@
   if (GetOpenTranscriptInEditor() && Host::IsInteractiveGraphicSession()) {
 const FileSpec file_spec;
 error = file->GetFileSpec(const_cast(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;
Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -325,12 +325,35 @@
 
 #endif // TARGET_OS_OSX
 
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
-uint32_t line_no) {
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+   uint32_t line_no) {
 #if !TARGET_OS_OSX
-  return false;
+  return llvm::formErrorCode(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() ? "" : 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 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;
@@ -340,18 +363,7 @@
 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 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 err

[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd updated this revision to Diff 518003.
kuilpd added a comment.

Made an ABI method that returns a stack frame size for the target.

Removed the condition for alignment. The default value doesn't matter too much 
even for a 16-bit target, no need to add another ABI method just for this. The 
programmer can use the settings to change it if necessary.


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

https://reviews.llvm.org/D149262

Files:
  lldb/include/lldb/Target/ABI.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Expression/IRMemoryMap.cpp
  lldb/source/Expression/LLVMUserExpression.cpp
  lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td

Index: lldb/source/Target/TargetProperties.td
===
--- lldb/source/Target/TargetProperties.td
+++ lldb/source/Target/TargetProperties.td
@@ -24,6 +24,15 @@
 DefaultUnsignedValue<5>,
 Desc<"The maximum amount of errors to emit while parsing an expression. "
  "A value of 0 means to always continue parsing if possible.">;
+  def ExprAllocAddress: Property<"expr-alloc-address", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Start address within the process address space of memory allocation for expression evaluation.">;
+  def ExprAllocSize: Property<"expr-alloc-size", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Amount of memory in bytes to allocate for expression evaluation.">;
+  def ExprAllocAlign: Property<"expr-alloc-align", "UInt64">,
+DefaultUnsignedValue<0>,
+Desc<"Alignment for each memory allocation for expression evaluation.">;
   def PreferDynamic: Property<"prefer-dynamic-value", "Enum">,
 DefaultEnumValue<"eDynamicDontRunTarget">,
 EnumValues<"OptionEnumValues(g_dynamic_value_types)">,
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4587,6 +4587,24 @@
   nullptr, idx, g_target_properties[idx].default_uint_value);
 }
 
+uint64_t TargetProperties::GetExprAllocAddress() const {
+  const uint32_t idx = ePropertyExprAllocAddress;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocSize() const {
+  const uint32_t idx = ePropertyExprAllocSize;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
+uint64_t TargetProperties::GetExprAllocAlign() const {
+  const uint32_t idx = ePropertyExprAllocAlign;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+  nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
 bool TargetProperties::GetBreakpointsConsultPlatformAvoidList() {
   const uint32_t idx = ePropertyBreakpointUseAvoidList;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
===
--- lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
+++ lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
@@ -55,6 +55,8 @@
   const lldb_private::RegisterInfo *
   GetRegisterInfoArray(uint32_t &count) override;
 
+  uint64_t GetStackFrameSize() override { return 512; }
+
   //--
   // Static Functions
   //--
Index: lldb/source/Expression/LLVMUserExpression.cpp
===
--- lldb/source/Expression/LLVMUserExpression.cpp
+++ lldb/source/Expression/LLVMUserExpression.cpp
@@ -23,6 +23,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
@@ -34,6 +35,7 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+using namespace lldb;
 using namespace lldb_private;
 
 char LLVMUserExpression::ID;
@@ -333,9 +335,17 @@
 if (m_can_interpret && m_stack_frame_bottom == LLDB_INVALID_ADDRESS) {
   Status alloc_error;
 
-  auto arch = target->GetArchitecture().GetTriple().getArch();
-  const size_t stack_frame_size =
-  arch == llvm::Triple::msp430 ? 512 : 512 * 1024;
+  size_t stack_frame_size = target->GetExprAllocSize();
+  if (stack_frame_size == 0) {
+Process *process_sp;
+ABISP abi_sp;
+if ((process_sp = exe_ctx.GetProcessPtr()) &&
+(abi_sp = process_sp->GetABI())) {
+  stack_frame_size = abi_sp->GetStackFrameSize();
+} else {
+  stack_frame_size = 512 * 1024;
+}
+  }
 
   const bool zero_memory = false;
 
Index: lldb/source/Expres

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  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);

bulbazord wrote:
> JDevlieghere wrote:
> > bulbazord wrote:
> > > I know you're preserving existing behavior here (or rather, fixing its 
> > > faulty implementation), but I think it would be nice if you didn't have 
> > > to restart your session and add an environment variable to get this 
> > > working with an existing debug session. Or maybe make it a setting?
> > I think we're trying to mimic the `EDITOR` and `VISUAL` environment 
> > variables but I agree a setting would be better. I can add that in a 
> > follow-up. Which one should take preference if you have both?
> I would say a setting should probably take priority? That way you can change 
> it during your session if desired.
If both are set and not the same, maybe we should flag that.  Maybe we can 
check when the setting is set, and if the environment variable is also set and 
different, we can warn at that point.  

But the intention of the setting is to change what the current behavior is, no 
matter how we got to the current behavior, so the setting should take 
precedence.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+

JDevlieghere wrote:
> bulbazord wrote:
> > nit: Move log line below checking if the file_path is empty? If the 
> > file_spec is empty we may see strange log lines like "Sending :0 to 
> > external editor" which is just noise.
> I actually did that on purpose, so we could tell from the logs that it's 
> empty. It didn't seem worth adding a whole separate log line for, but if you 
> think `:10` looks weird, I'm happy to do it. 
I think a separate log line would be easier to read for somebody not familiar 
with this codepath. It would be confusing otherwise. Something like 
`"OpenFileInExternalEditor called with empty path"`? Or maybe you can keep the 
log line but change it to:

```
LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path.empty() ? 
"" : file_path, line_no);



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  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);

JDevlieghere wrote:
> bulbazord wrote:
> > I know you're preserving existing behavior here (or rather, fixing its 
> > faulty implementation), but I think it would be nice if you didn't have to 
> > restart your session and add an environment variable to get this working 
> > with an existing debug session. Or maybe make it a setting?
> I think we're trying to mimic the `EDITOR` and `VISUAL` environment variables 
> but I agree a setting would be better. I can add that in a follow-up. Which 
> one should take preference if you have both?
I would say a setting should probably take priority? That way you can change it 
during your session if desired.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
   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());
 

JDevlieghere wrote:
> bulbazord wrote:
> > Should we exit early if we don't have `g_app_fsref` filled in? The 
> > `application` field of `app_params` won't be filled in so what will 
> > `LSOpenURLsWithRole` do?
> No, the application is optional. If none is specified we use the default. You 
> can't pass a default constructor one though, it has to be NULL, which is why 
> we have the check here.
LSOpenURLsWithRole will use the default application for the extension that the 
file path we pass it has, however, that's likely to be a different app than the 
one the user dialed up with LLDB_EXTERNAL_EDITOR.  

If the user told us to use an external editor at some path, but we can't 
actually find that application, then letting the default application open seems 
wrong.  Ideally, we would raise an error saying "External Editor: ... not 
found" but it doesn't look like there's any way to report errors from this code.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:420
+  error = ::LSOpenURLsWithRole(
+  /*inURLs=*/cf_array.get(), /*inRole=*/kLSRolesAll,
+  /*inAEParam=*/&file_and_line_desc,

I guess we could narrow down the `inRole` argument: 
https://developer.apple.com/documentation/coreservices/lsrolesmask/klsroleseditor?language=objc`


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:343-344
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser file_URL(::CFURLCreateWithFileSystemPath(
+  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+

bulbazord wrote:
> Could you document what the NULL and false refer to? I think they're for 
> allocator and isDirectory or something like that.
+1



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
   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());
 

JDevlieghere wrote:
> bulbazord wrote:
> > Should we exit early if we don't have `g_app_fsref` filled in? The 
> > `application` field of `app_params` won't be filled in so what will 
> > `LSOpenURLsWithRole` do?
> No, the application is optional. If none is specified we use the default. You 
> can't pass a default constructor one though, it has to be NULL, which is why 
> we have the check here.
Looking at the documentation 
(https://developer.apple.com/documentation/coreservices/1448184-lsopenurlswithrole),
 if `app_params.application` is `NULL`, `LSOpenURLsWithRole` makes use of 
`kLSRolesAll` 
(https://developer.apple.com/documentation/coreservices/lsrolesmask?language=objc)
 which I believe open the default application for that file.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via Phabricator via lldb-commits]

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



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:335
+
+  const std::string file_path = file_spec.GetPath();
+

mib wrote:
> I guess this doesn't need to be a `const std::string`, a `llvm::StringRef` 
> would have been nice but looking at `SBFileSpec::GetPath`, there is 
> unfortunately no overload for that.
Given how FileSpecs stores path components, you basically always need an owning 
string. 



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:385
 
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+  static std::optional g_app_fsref;
+  static std::once_flag g_once_flag;

mib wrote:
> why is this static ?
Because it is cached across calls. 


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via Phabricator via lldb-commits]

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



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+

bulbazord wrote:
> nit: Move log line below checking if the file_path is empty? If the file_spec 
> is empty we may see strange log lines like "Sending :0 to external editor" 
> which is just noise.
I actually did that on purpose, so we could tell from the logs that it's empty. 
It didn't seem worth adding a whole separate log line for, but if you think 
`:10` looks weird, I'm happy to do it. 



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  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);

bulbazord wrote:
> I know you're preserving existing behavior here (or rather, fixing its faulty 
> implementation), but I think it would be nice if you didn't have to restart 
> your session and add an environment variable to get this working with an 
> existing debug session. Or maybe make it a setting?
I think we're trying to mimic the `EDITOR` and `VISUAL` environment variables 
but I agree a setting would be better. I can add that in a follow-up. Which one 
should take preference if you have both?



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
   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());
 

bulbazord wrote:
> Should we exit early if we don't have `g_app_fsref` filled in? The 
> `application` field of `app_params` won't be filled in so what will 
> `LSOpenURLsWithRole` do?
No, the application is optional. If none is specified we use the default. You 
can't pass a default constructor one though, it has to be NULL, which is why we 
have the check here.


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[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 with some comments




Comment at: lldb/source/Host/macosx/objcxx/Host.mm:335
+
+  const std::string file_path = file_spec.GetPath();
+

I guess this doesn't need to be a `const std::string`, a `llvm::StringRef` 
would have been nice but looking at `SBFileSpec::GetPath`, there is 
unfortunately no overload for that.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:385
 
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+  static std::optional g_app_fsref;
+  static std::once_flag g_once_flag;

why is this static ?


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

LGTM overall. A few nits but overall an excellent cleanup! :)




Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+

nit: Move log line below checking if the file_path is empty? If the file_spec 
is empty we may see strange log lines like "Sending :0 to external editor" 
which is just noise.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:343-344
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser file_URL(::CFURLCreateWithFileSystemPath(
+  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+

Could you document what the NULL and false refer to? I think they're for 
allocator and isDirectory or something like that.



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  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);

I know you're preserving existing behavior here (or rather, fixing its faulty 
implementation), but I think it would be nice if you didn't have to restart 
your session and add an environment variable to get this working with an 
existing debug session. Or maybe make it a setting?



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
   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());
 

Should we exit early if we don't have `g_app_fsref` filled in? The 
`application` field of `app_params` won't be filled in so what will 
`LSOpenURLsWithRole` do?


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

https://reviews.llvm.org/D149472

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

[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Jonas Devlieghere via Phabricator via lldb-commits]

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

This patch refactors the macOS implementation of `OpenFileInExternalEditor`:

- Fix `AppleEvent` memory leak
- Fix caching of `LLDB_EXTERNAL_EDITOR`
- Fix (speculatively) crash when CFURL is NULL.
- Improve readability
- Improve documentation
- Improve logging

rdar://108633464


https://reviews.llvm.org/D149472

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

Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -330,7 +330,23 @@
 #if !TARGET_OS_OSX
   return false;
 #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, line_no);
+
+  if (file_path.empty())
+return false;
+
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser file_URL(::CFURLCreateWithFileSystemPath(
+  NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+
+  if (!file_URL.get())
+return false;
+
+  // Create a new Apple Event descriptor.
   typedef struct {
 int16_t reserved0; // must be zero
 int16_t fLineNumber;
@@ -340,18 +356,7 @@
 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 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)
@@ -362,60 +367,62 @@
   };
 
   AEKeyDesc file_and_line_desc;
-
-  error = ::AECreateDesc(typeUTF8Text, &file_and_line_info,
- sizeof(file_and_line_info),
- &(file_and_line_desc.descContent));
+  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) {
-LLDB_LOGF(log, "Error creating AEDesc: %ld.\n", error);
+LLDB_LOG(log, "Creating Apple Event descriptor failed: error {0}", error);
 return false;
   }
 
-  file_and_line_desc.descKey = keyAEPosition;
+  // Deallocate the descriptor on exit.
+  auto on_exit = llvm::make_scope_exit(
+  [&]() { AEDisposeDesc(&(file_and_line_desc.descContent)); });
 
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+  static std::optional g_app_fsref;
+  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);
+  error = ::LSFindApplicationForInfo(
+  /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
+  /*inName=*/editor_name.get(), /*outAppRef=*/&app_fsref,
+  /*outAppURL=*/NULL);
+  if (error == noErr) {
+g_app_fsref = app_fsref;
+  } else {
+LLDB_LOG(log, "Could not find external editor \"{0}\": error {1}",
+ external_editor, 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);

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

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGade3c6a6a88e: Host: generalise `GetXcodeSDKPath` (authored 
by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D149397?vs=517764&id=517959#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149397

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -57,7 +57,8 @@
 #if defined(__APPLE__)
 TEST_F(HostInfoTest, GetXcodeSDK) {
   auto get_sdk = [](std::string sdk, bool error = false) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK(std::move(sdk))});
 if (!error) {
   EXPECT_TRUE((bool)sdk_path_or_err);
   return *sdk_path_or_err;
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -124,7 +124,8 @@
   }
 
   // Use the default SDK as a fallback.
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+  auto sdk_path_or_err =
+  HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK::GetAnyMacOS()});
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
   toString(sdk_path_or_err.takeError()));
Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -284,7 +284,8 @@
   std::string secondary) {
   llvm::StringRef sdk;
   auto get_sdk = [&](std::string sdk) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK(std::move(sdk))});
 if (!sdk_path_or_err) {
   Debugger::ReportError("Error while searching for Xcode SDK: " +
 toString(sdk_path_or_err.takeError()));
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -338,7 +338,8 @@
   }
 }
 
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(SDKOptions{XcodeSDK::GetAnyMacOS()});
 if (!sdk_path_or_err) {
   Log *log = GetLog(LLDBLog::Host);
   LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
@@ -519,7 +520,7 @@
   return path;
 }
 
-llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+llvm::Expected HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
   struct ErrorOrPath {
 std::string str;
 bool is_error;
@@ -530,6 +531,11 @@
   std::lock_guard guard(g_sdk_path_mutex);
   LLDB_SCOPED_TIMER();
 
+  if (!options.XcodeSDK)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "XCodeSDK not specified");
+  XcodeSDK sdk = *options.XcodeSDK;
+
   auto key = sdk.GetString();
   auto it = g_sdk_path.find(key);
   if (it != g_sdk_path.end()) {
Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -1607,8 +1607,8 @@
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
   llvm::StringRef sysroot) {
-  XcodeSDK sdk(sdk_name.str());
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+  auto sdk_path_or_err =
+  HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk_name.str()});
 
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
Index: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -31,7 +31,7 @@
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
-  static llvm::Expected GetXcodeSDKPath(Xco

[Lldb-commits] [lldb] ade3c6a - Host: generalise `GetXcodeSDKPath`

[Saleem Abdulrasool via lldb-commits]

Author: Saleem Abdulrasool
Date: 2023-04-28T09:30:59-07:00
New Revision: ade3c6a6a88ed3a9b06c076406f196da9d3cc1b9

URL: 
https://github.com/llvm/llvm-project/commit/ade3c6a6a88ed3a9b06c076406f196da9d3cc1b9
DIFF: 
https://github.com/llvm/llvm-project/commit/ade3c6a6a88ed3a9b06c076406f196da9d3cc1b9.diff

LOG: Host: generalise `GetXcodeSDKPath`

This generalises the GetXcodeSDKPath hook to a GetSDKRoot path which
will be re-used for the Windows support to compute a language specific
SDK path on the platform. Because there may be other options that we
wish to use to compute the SDK path, sink the XcodeSDK parameter into
a structure which can pass a disaggregated set of options. Furthermore,
optionalise the parameter as Xcode is not available for all platforms.

Differential Revision: https://reviews.llvm.org/D149397
Reviewed By: JDevlieghere

Added: 


Modified: 
lldb/include/lldb/Host/HostInfoBase.h
lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
lldb/source/Core/Module.cpp
lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
lldb/unittests/Host/HostInfoTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/HostInfoBase.h 
b/lldb/include/lldb/Host/HostInfoBase.h
index 42f71d91f9bf9..28f809398d5b4 100644
--- a/lldb/include/lldb/Host/HostInfoBase.h
+++ b/lldb/include/lldb/Host/HostInfoBase.h
@@ -31,6 +31,23 @@ struct SharedCacheImageInfo {
   lldb::DataBufferSP data_sp;
 };
 
+namespace {
+struct HostInfoError : public llvm::ErrorInfo {
+  static char ID;
+  const std::string message_;
+
+  HostInfoError(const std::string message) : message_(std::move(message)) {}
+
+  void log(llvm::raw_ostream &OS) const override { OS << "HostInfoError"; }
+
+  std::error_code convertToErrorCode() const override {
+return llvm::inconvertibleErrorCode();
+  }
+};
+
+char HostInfoError::ID = 0;
+} // namespace
+
 class HostInfoBase {
 private:
   // Static class, unconstructable.
@@ -108,10 +125,14 @@ class HostInfoBase {
 
   static FileSpec GetXcodeContentsDirectory() { return {}; }
   static FileSpec GetXcodeDeveloperDirectory() { return {}; }
-  
-  /// Return the directory containing a specific Xcode SDK.
-  static llvm::Expected GetXcodeSDKPath(XcodeSDK sdk) {
-return "";
+
+  struct SDKOptions {
+std::optional XcodeSDK;
+  };
+
+  /// Return the directory containing something like a SDK (reused for Swift).
+  static llvm::Expected GetSDKRoot(SDKOptions options) {
+return llvm::make_error("cannot determine SDK root");
   }
 
   /// Return information about module \p image_name if it is loaded in

diff  --git a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h 
b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
index 0402509cfa261..74d979d965a73 100644
--- a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -31,7 +31,7 @@ class HostInfoMacOSX : public HostInfoPosix {
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
-  static llvm::Expected GetXcodeSDKPath(XcodeSDK sdk);
+  static llvm::Expected GetSDKRoot(SDKOptions options);
 
   /// Shared cache utilities
   static SharedCacheImageInfo

diff  --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 17d8043852ab7..6293cc7853f53 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1607,8 +1607,8 @@ std::optional 
Module::RemapSourceFile(llvm::StringRef path) const {
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
   llvm::StringRef sysroot) {
-  XcodeSDK sdk(sdk_name.str());
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+  auto sdk_path_or_err =
+  HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk_name.str()});
 
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +

diff  --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm 
b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
index 5a39ed370747a..6569013044513 100644
--- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -338,7 +338,8 @@ static void ParseOSVersion(llvm::VersionTuple &version, 
NSString *Key) {
   }
 }
 
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(SDKOptions{XcodeSDK::GetAnyMacOS()});
 if (!sdk_path_or_err) {
   Log *log = GetLog(LLDBLog::Host);
   LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
@@ -519,7 +520,7 @@ static void ParseOSVersion(llvm::VersionTuple &version, 
NSString *Key) {
   return path;
 }
 
-llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+llvm::Expected HostInfoMacOSX::GetSDKRoot(SDKOptions options) 
{
   struct E

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for all the helpful comments, I'll update the patch.




Comment at: 
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:833-835
+std::vector
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t user_addr,
+   nub_size_t user_size) {

JDevlieghere wrote:
> Do you ever expect this to return more than two watchpoints? Seems like this 
> could be a `struct` that holds two optional `WatchpointSpec`s. I don't feel 
> strongly about it, but it looks like you never iterate over the list and the 
> way you have to check after the recursive call is a bit awkward. 
> 
> edit: nvm, it looks like you do actually iterate over them in 
> `EnableHardwareWatchpoint`
I was thinking about how this current scheme only ever splits 1 watchpoint into 
2, but a future design that could expand a user requested watch into a larger 
number of hardware watchpoints would expand it further.  If a user asks to 
watch a 32 byte object, and the target only supports 8 byte watchpoints, and 
the object is doubleword aligned, we could watch it with 4 hardware 
watchpoints.  The older code in debugserver was written in terms of "one or 
two" but I switched to a vector of hardware implementable watchpoints that may 
expand as we evaluate the hardware capabilities of the target/stub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

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

[Lldb-commits] [PATCH] D147831: [lldb-vscode] Implement RestartRequest

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/API/SBProcess.cpp:772
 
-  return (event.GetBroadcasterClass() == SBProcess::GetBroadcasterClass()) &&
- !EventIsStructuredDataEvent(event);
+  return Process::ProcessEventData::GetEventDataFromEvent(event.get()) !=
+ nullptr;

+1



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1976
+if (state != lldb::eStateConnected) {
+  process.Kill();
+}

Kill actually detaches if lldb originally attached to the debuggee. The actual 
implementation says

```
  /// Kills the process and shuts down all threads that were spawned to track
  /// and monitor the process.
  ///
  /// This function is not meant to be overridden by Process subclasses.
  ///
  /// \param[in] force_kill
  /// Whether lldb should force a kill (instead of a detach) from
  /// the inferior process.  Normally if lldb launched a binary and
  /// Destory is called, lldb kills it.  If lldb attached to a
  /// running process and Destory is called, lldb detaches.  If
  /// this behavior needs to be over-ridden, this is the bool that
  /// can be used.
  ///
  /// \return
  /// Returns an error object.
  Status Destroy(bool force_kill);
```
You could have the restart command reattach to the process instead of failing 
if the user originally attached. 

What do you think of this?


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

https://reviews.llvm.org/D147831

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

[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Thorsten via Phabricator via lldb-commits]

tschuett added inline comments.



Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:856
+  // user_size == 16 -> aligned_size == 16
+  aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+

JDevlieghere wrote:
> Beautiful. Once we have C++20 we can use `std::bit_ceil` 
> (https://en.cppreference.com/w/cpp/numeric/bit_ceil)
Is the builtin available on all supported platforms and compilers? There are 
some alignment functions in MathExtras.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

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

[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D146058#4224001 , @sgraenitz wrote:

> In D146058#4223575 , @labath wrote:
>
>> What kind of setup is necessary to make that work? If it's not too 
>> complicated, I think we could make something work.
>
> Thanks, that'd be awesome!
>
> The last official release is from 2020, so for the time being it seems best 
> to build and install from TOT once -- @theraven please correct me if I am 
> wrong:
>
>   > git clone https://github.com/gnustep/libobjc2
>   > cd libobjc2
>   > git submodule init && git submodule update
>   > CC=clang-15 CXX=clang++-15 cmake -Bbuild -GNinja -DTESTS=On .
>   > cd build
>   > ninja
>   ...
>   > ctest
>   ...
>   100% tests passed, 0 tests failed out of 198
>   > ninja install
>   ...
>   -- Installing: /usr/local/lib/libobjc.so.4.6
>   -- Installing: /usr/local/lib/libobjc.so
>   ...
>
> Then apply this patch and re-run CMake. The new test should popup and pass:

I'm sorry for the delay. Building a package from source is slightly more that 
what I have time for right now. As you may have noticed, I don't have much time 
for LLDB work right now, and I'm trying to keep the buildbot a vanilla debian 
install so it's easy to reproduce its setup (both for myself and other 
developers).

> How can I change this to support the well-behaved bot? Add 
> `-DLLDB_TEST_OBJC_GNUSTEP=Off`? Or should it be off by default and enabled on 
> demand?

That's not entirely what I was referring to. What I fear is the following 
situation. A random developer makes a random patch that happens to break 
gnustep support. That developer cannot debug that issue locally (cannot or 
doesn't know how to build gnustep from source), so someone has to help him 
figure out the problem. I don't want to be the person doing that. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146058

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

[Lldb-commits] [PATCH] D147831: [lldb-vscode] Implement RestartRequest

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks ok-ish, though I don't feel entirely comfortable approving lldb-vscode 
changes. @wallace, do you have any thoughts on this?




Comment at: lldb/tools/lldb-vscode/VSCode.h:152
   bool is_attach;
+  // The process event thread normally responds to process exited events by
+  // shutting down the entire adapter. When we're restarting, we keep the id of

So, it's not possible to restart a process if it has already terminated? I.e., 
if the debugged process unexpectedly exits, the user will have to restart 
lldb-vscode from scratch (so he can e.g., set a breakpoint in before the exit).



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1983
+  g_vsc.debugger.SetAsync(true);
+  LaunchProcess(*g_vsc.last_launch_or_attach_request);
+

I have no idea if that's a good idea or not, but I'm wondering if, instead of 
from the last attach request, we should be taking the arguments from lldb. So 
that if the user say changes the `target.run-args` setting, then the new 
restart request will take that into account...


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

https://reviews.llvm.org/D147831

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

[Lldb-commits] [PATCH] D147642: [lldb][ObjectFileELF] Support AArch32 in ApplyRelocations

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2663
+// the actual range check below.
+if (addend < 0 && static_cast(std::abs(addend)) > value) {
+  LLDB_LOGF(log, "Debug info relocation overflow: 0x%" PRIx64,





Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2660
+// Implicit addend is stored inline as a signed value.
+int32_t addend = *reinterpret_cast(dst);
+// The sum must be positive. This extra check prevents UB from overflow in

sgraenitz wrote:
> IIUC we'd want to account for an endianness difference between debugger and 
> target (in theory). However, non of the other cases seems to do it, so I 
> didn't start with it either.
We probably should.
What we also should do (and what other cases seem to get mostly right) is avoid 
dereferencing type-punned pointers (use memcpy to read).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147642

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

[Lldb-commits] [PATCH] D149214: [lldb] Speed up DebugAbbrev parsing

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I guess the most efficient (performance- and memory-wise) approach would be to 
have a global (well, scoped to a DWARFUnit or something) array of 
DWARFAttribute objects, and have the individual abbreviations just store 
pointers/indexes to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149214

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

[Lldb-commits] [PATCH] D148662: [lldb] Make the libcxx unique_ptr prettyprinter support custom deleters.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:52
+ValueObject &pair) {
+  ValueObjectSP value = pair.GetChildAtIndex(0, true)->GetChildMemberWithName(
+  ConstString("__value_"), true);

We should check that `pair.GetChildAtIndex(0, true)` returns a valid value 
before dereferencing it, so we don't crash if the definition of the 
comppressed_pair type is missing or something like



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:65
+  ValueObjectSP value;
+  if (pair.GetNumChildren() > 1)
+value = pair.GetChildAtIndex(1, true)->GetChildMemberWithName(

Similarly, here (I'm don't think that `GetNumChildren()>1` actually guarantees 
that the GetChildAtIndex(1) will return a valid object).



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:725
 
-  m_value_ptr_sp = GetValueOfLibCXXCompressedPair(*ptr_sp);
+  m_value_ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
+  ValueObjectSP deleter_sp = GetSecondValueOfLibCXXCompressedPair(*ptr_sp);

unrelated, but it might be nice to give a better name to the pointed-to value 
as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148662

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

[Lldb-commits] [PATCH] D147642: [lldb][ObjectFileELF] Support AArch32 in ApplyRelocations

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147642

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