[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

2023-08-11 Thread Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits 
RamNalamothu added a comment.

I think I have addressed all the review feedback. Ok to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156086

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

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

The lldb side looks good to me, and I think you've addressed the llvm parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156086

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

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added inline comments.



Comment at: lldb/source/Host/posix/PipePosix.cpp:69
   PipeBase::operator=(std::move(pipe_posix));
-  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
-  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+  std::lock_guard guard(m_lock);
+  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();

bulbazord wrote:
> I think this lock needs to be at the beginning. Scenario:
> 
> Thread 1 calls the move assign operator and performs the base move assign. 
> Gets unscheduled before acquiring lock.
> Thread 2 calls the move assign operator and performs the base move assign. 
> Acquires the lock, fills in `m_fds`, and returns.
> Thread 1 is scheduled again, grabs the lock, and fills in `m_fds`.
> `this` will have its `PipeBase` members filled in by the Thread 2 call while 
> `m_fds` will be filled in by the Thread 1 call. Granted, one thread may be 
> surprised that suddenly the thing didn't get moved in as expected, but at 
> least `this` will be in a consistent state.
> 
> I think you also need to lock the mutex of `pipe_posix` at the same time. 
> Imagine Thread 1 is trying to access something from `pipe_posix` while Thread 
> 2 is trying to move it into another `PipePosix` object. (Not sure how likely 
> this scenario is but I'd rather be safe than sorry).
> I think you also need to lock the mutex of pipe_posix at the same time. 
> Imagine Thread 1 is trying to access something from pipe_posix while Thread 2 
> is trying to move it into another PipePosix object. (Not sure how likely this 
> scenario is but I'd rather be safe than sorry).

Yeah I thought about this too, since `pipe_posix` is an rvalue reference I was 
thinking this shouldn't be necessary, but you're right, I'll lock it anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

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

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 updated this revision to Diff 549454.
augusto2112 added a comment.

Lock earlier in operator=


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

Files:
  lldb/include/lldb/Host/posix/PipePosix.h
  lldb/source/Host/posix/PipePosix.cpp

Index: lldb/source/Host/posix/PipePosix.cpp
===
--- lldb/source/Host/posix/PipePosix.cpp
+++ lldb/source/Host/posix/PipePosix.cpp
@@ -65,16 +65,19 @@
 pipe_posix.ReleaseWriteFileDescriptor()} {}
 
 PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
+  std::scoped_lock guard(m_lock, pipe_posix.m_lock);
+
   PipeBase::operator=(std::move(pipe_posix));
-  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
-  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();
+  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptorUnlocked();
   return *this;
 }
 
 PipePosix::~PipePosix() { Close(); }
 
 Status PipePosix::CreateNew(bool child_processes_inherit) {
-  if (CanRead() || CanWrite())
+  std::lock_guard guard(m_lock);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status(EINVAL, eErrorTypePOSIX);
 
   Status error;
@@ -87,7 +90,7 @@
 if (!child_processes_inherit) {
   if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
 error.SetErrorToErrno();
-Close();
+CloseUnlocked();
 return error;
   }
 }
@@ -103,7 +106,8 @@
 }
 
 Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  std::lock_guard guard(m_lock);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   Status error;
@@ -140,7 +144,8 @@
 
 Status PipePosix::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  std::lock_guard guard(m_lock);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_RDONLY | O_NONBLOCK;
@@ -161,7 +166,8 @@
 PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
bool child_process_inherit,
const std::chrono::microseconds &timeout) {
-  if (CanRead() || CanWrite())
+  std::lock_guard guard(m_lock);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_WRONLY | O_NONBLOCK;
@@ -171,7 +177,7 @@
   using namespace std::chrono;
   const auto finish_time = Now() + timeout;
 
-  while (!CanWrite()) {
+  while (!CanWriteUnlocked()) {
 if (timeout != microseconds::zero()) {
   const auto dur = duration_cast(finish_time - Now()).count();
   if (dur <= 0)
@@ -196,25 +202,54 @@
   return Status();
 }
 
-int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; }
+int PipePosix::GetReadFileDescriptor() const {
+  std::lock_guard guard(m_lock);
+  return GetReadFileDescriptorUnlocked();
+}
+
+int PipePosix::GetReadFileDescriptorUnlocked() const {
+  return m_fds[READ];
+}
+
+int PipePosix::GetWriteFileDescriptor() const {
+  std::lock_guard guard(m_lock);
+  return GetWriteFileDescriptorUnlocked();
+}
 
-int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; }
+int PipePosix::GetWriteFileDescriptorUnlocked() const {
+  return m_fds[WRITE];
+}
 
 int PipePosix::ReleaseReadFileDescriptor() {
+  std::lock_guard guard(m_lock);
+  return ReleaseReadFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseReadFileDescriptorUnlocked() {
   const int fd = m_fds[READ];
   m_fds[READ] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 int PipePosix::ReleaseWriteFileDescriptor() {
+  std::lock_guard guard(m_lock);
+  return ReleaseWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseWriteFileDescriptorUnlocked() {
   const int fd = m_fds[WRITE];
   m_fds[WRITE] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 void PipePosix::Close() {
-  CloseReadFileDescriptor();
-  CloseWriteFileDescriptor();
+  std::lock_guard guard(m_lock);
+  CloseUnlocked();
+}
+
+void PipePosix::CloseUnlocked() {
+  CloseReadFileDescriptorUnlocked();
+  CloseWriteFileDescriptorUnlocked();
 }
 
 Status PipePosix::Delete(llvm::StringRef name) {
@@ -222,22 +257,41 @@
 }
 
 bool PipePosix::CanRead() const {
+  std::lock_guard guard(m_lock);
+  return CanReadUnlocked();
+}
+
+bool PipePosix::CanReadUnlocked() const {
   return m_fds[READ] != PipePosix::kInvalidDescriptor;
 }
 
 bool PipePosix::CanWrite() const {
+  std::lock_guard guard(m_lock);
+  return CanWriteUnlocked();
+}
+
+bool PipePosix::CanWriteUnlocked() const {
   return m_fds[WRITE] != PipePosix::kInvalidDescriptor;
 }
 
 void PipePosix::CloseReadFileDescriptor() {
-  if (CanRead()) {
+  std::lock_guard guard(m_lock);
+  CloseReadFileDescriptorUnlocked();
+}
+void Pi

[Lldb-commits] [PATCH] D157640: [lldb] Improve error message when trying to debug a non-debuggable process

2023-08-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D157640#4578198 , @jingham wrote:

> In this function we have the path to the binary.  We could spawn `codesign -d 
> -entitlements -` and then we would know whether it had that entitlement.
>
> Maybe that's more work than you wanted to do here, however.

Yeah I'm not sure that being able to be more precise is a huge improvement over 
the current error message. If we still see a lot of people struggling with this 
we can easily reconsider.


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

https://reviews.llvm.org/D157640

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

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

Shouldn't we have two murexes, one for the read FD and one for the write FD? 
The current approach is overly strict: it preventing concurrent reads and 
writes because `ReadWithTimeout` and `Write` are locking the same mutex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

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

[Lldb-commits] [lldb] be237b7 - [lldb] Improve error message when trying to debug a non-debuggable process

2023-08-11 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2023-08-11T10:47:00-07:00
New Revision: be237b76da7e29debd9cf1e557235f945b92cead

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

LOG: [lldb] Improve error message when trying to debug a non-debuggable process

On the Swift forums, people are disabling SIP in order to debug process
that are missing the get-task-allow entitlement. Improve the error to
give developers a hint at the potential issues.

rdar://113704200

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

Added: 


Modified: 
lldb/tools/debugserver/source/DNB.cpp

Removed: 




diff  --git a/lldb/tools/debugserver/source/DNB.cpp 
b/lldb/tools/debugserver/source/DNB.cpp
index 2a9a486f3e134e..f6c1130fdd8054 100644
--- a/lldb/tools/debugserver/source/DNB.cpp
+++ b/lldb/tools/debugserver/source/DNB.cpp
@@ -382,11 +382,22 @@ nub_process_t DNBProcessLaunch(
 if (err_str && err_len > 0) {
   if (launch_err.AsString()) {
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i (%s)", pid,
+   "failed to get the task for process %i: %s", pid,
launch_err.AsString());
   } else {
+
+const char *ent_name =
+#if TARGET_OS_OSX
+  "com.apple.security.get-task-allow";
+#else
+  "get-task-allow";
+#endif
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i", pid);
+   "failed to get the task for process %i: this likely "
+   "means the process cannot be debugged, either because "
+   "it's a system process or because the process is "
+   "missing the %s entitlement.",
+   pid, ent_name);
   }
 }
   } else {



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

[Lldb-commits] [PATCH] D157640: [lldb] Improve error message when trying to debug a non-debuggable process

2023-08-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbe237b76da7e: [lldb] Improve error message when trying to 
debug a non-debuggable process (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D157640?vs=549119&id=549459#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157640

Files:
  lldb/tools/debugserver/source/DNB.cpp


Index: lldb/tools/debugserver/source/DNB.cpp
===
--- lldb/tools/debugserver/source/DNB.cpp
+++ lldb/tools/debugserver/source/DNB.cpp
@@ -382,11 +382,22 @@
 if (err_str && err_len > 0) {
   if (launch_err.AsString()) {
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i (%s)", pid,
+   "failed to get the task for process %i: %s", pid,
launch_err.AsString());
   } else {
+
+const char *ent_name =
+#if TARGET_OS_OSX
+  "com.apple.security.get-task-allow";
+#else
+  "get-task-allow";
+#endif
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i", pid);
+   "failed to get the task for process %i: this likely "
+   "means the process cannot be debugged, either because "
+   "it's a system process or because the process is "
+   "missing the %s entitlement.",
+   pid, ent_name);
   }
 }
   } else {


Index: lldb/tools/debugserver/source/DNB.cpp
===
--- lldb/tools/debugserver/source/DNB.cpp
+++ lldb/tools/debugserver/source/DNB.cpp
@@ -382,11 +382,22 @@
 if (err_str && err_len > 0) {
   if (launch_err.AsString()) {
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i (%s)", pid,
+   "failed to get the task for process %i: %s", pid,
launch_err.AsString());
   } else {
+
+const char *ent_name =
+#if TARGET_OS_OSX
+  "com.apple.security.get-task-allow";
+#else
+  "get-task-allow";
+#endif
 ::snprintf(err_str, err_len,
-   "failed to get the task for process %i", pid);
+   "failed to get the task for process %i: this likely "
+   "means the process cannot be debugged, either because "
+   "it's a system process or because the process is "
+   "missing the %s entitlement.",
+   pid, ent_name);
   }
 }
   } else {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added a comment.

The idea seems fine to me. A few nits and comments, otherwise LGTM.




Comment at: lldb/source/API/SBTarget.cpp:1517
+  if (Symbols::DownloadObjectAndSymbolFile(*module_spec.m_opaque_up, error,
+   true)) {
+if (FileSystem::Instance().Exists(

nit: add a comment next to `true` to indicate that it's for `copy_executable`



Comment at: lldb/source/API/SBTarget.cpp:1528-1529
+  // binary's architecture.
+  if (sb_module.IsValid() && !target_sp->GetArchitecture().IsValid() &&
+  sb_module.GetSP() && sb_module.GetSP()->GetArchitecture().IsValid())
+target_sp->SetArchitecture(sb_module.GetSP()->GetArchitecture());

nit: It's redundant to check `sb_module.GetSP()` because `sb_module.IsValid()` 
already does that.



Comment at: 
lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py:39
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+).decode("utf-8")

I know this is already done in a few places but we should really use 
`llvm-dwarfdump` so this can be a more portable test...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157659

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

[Lldb-commits] [PATCH] D157667: Define qHostInfo and Mach-O LC_NOTE "addrable bits" methods to describe high and low memory addressing bits

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added inline comments.



Comment at: lldb/include/lldb/Symbol/ObjectFile.h:500-503
+  /// /param[out] lowmem_mask
+  /// Address mask to be used for low memory addresses.
+  ///
+  /// /param[out] highmem_mask

`/param` -> `\param`



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:917-918
+  lldb::addr_t hi_address_mask = ~((1ULL << high_mem_addressable_bits) - 
1);
+  SetCodeAddressMask(hi_address_mask);
+  SetDataAddressMask(hi_address_mask);
+}

These should be `SetHighmemCodeAddressMask` and `SetHighmemDataAddressMask` 
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157667

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

[Lldb-commits] [PATCH] D157667: Define qHostInfo and Mach-O LC_NOTE "addrable bits" methods to describe high and low memory addressing bits

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda updated this revision to Diff 549504.
jasonmolenda added a comment.

Fix the problems Alex found on review, thanks Alex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157667

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -509,10 +509,14 @@
 
   CleanupMemoryRegionPermissions();
 
-  addr_t address_mask = core_objfile->GetAddressMask();
-  if (address_mask != 0) {
-SetCodeAddressMask(address_mask);
-SetDataAddressMask(address_mask);
+  addr_t lo_addr_mask, hi_addr_mask;
+  if (core_objfile->GetAddressMask(lo_addr_mask, hi_addr_mask)) {
+SetCodeAddressMask(lo_addr_mask);
+SetDataAddressMask(lo_addr_mask);
+if (lo_addr_mask != hi_addr_mask) {
+  SetHighmemCodeAddressMask(hi_addr_mask);
+  SetHighmemDataAddressMask(hi_addr_mask);
+}
   }
   return error;
 }
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -899,10 +899,24 @@
  process_arch.GetTriple().getTriple());
   }
 
-  if (int addressable_bits = m_gdb_comm.GetAddressingBits()) {
-lldb::addr_t address_mask = ~((1ULL << addressable_bits) - 1);
+  uint32_t low_mem_addressable_bits, high_mem_addressable_bits;
+  m_gdb_comm.GetAddressingBits(low_mem_addressable_bits,
+   high_mem_addressable_bits);
+  // In case we were only given a low or high memory addressing bits,
+  // copy the value to the other.
+  if (low_mem_addressable_bits == 0)
+low_mem_addressable_bits = high_mem_addressable_bits;
+  if (high_mem_addressable_bits == 0)
+high_mem_addressable_bits = low_mem_addressable_bits;
+  if (low_mem_addressable_bits != 0) {
+lldb::addr_t address_mask = ~((1ULL << low_mem_addressable_bits) - 1);
 SetCodeAddressMask(address_mask);
 SetDataAddressMask(address_mask);
+if (low_mem_addressable_bits != high_mem_addressable_bits) {
+  lldb::addr_t hi_address_mask = ~((1ULL << high_mem_addressable_bits) - 1);
+  SetHighmemCodeAddressMask(hi_address_mask);
+  SetHighmemDataAddressMask(hi_address_mask);
+}
   }
 
   if (process_arch.IsValid()) {
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -237,7 +237,8 @@
 
   ArchSpec GetSystemArchitecture();
 
-  uint32_t GetAddressingBits();
+  void GetAddressingBits(uint32_t &lo_mem_addr_bits,
+ uint32_t &hi_mem_addr_bits);
 
   bool GetHostname(std::string &s);
 
@@ -580,7 +581,8 @@
   lldb::tid_t m_curr_tid_run = LLDB_INVALID_THREAD_ID;
 
   uint32_t m_num_supported_hardware_watchpoints = 0;
-  uint32_t m_addressing_bits = 0;
+  uint32_t m_low_mem_addressing_bits = 0;
+  uint32_t m_high_mem_addressing_bits = 0;
 
   ArchSpec m_host_arch;
   std::string m_host_distribution_id;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1265,7 +1265,15 @@
 if (!value.getAsInteger(0, pointer_byte_size))
   ++num_keys_decoded;
   } else if (name.equals("addressing_bits")) {
-if (!value.getAsInteger(0, m_addressing_bits))
+if (!value.getAsInteger(0, m_low_mem_addressing_bits)) {
+  m_high_mem_addressing_bits = m_low_mem_addressing_bits;
+  ++num_keys_decoded;
+}
+  } else if (name.equals("high_mem_addressing_bits")) {
+if (!value.getAsInteger(0, m_high_mem_addressing_bits))
+  ++num_keys_decoded;
+  } else if (name.equals("low_mem_addressing_bits")) {
+if (!value.getAsInteger(0, m_low_mem_addressing_bits))
   ++num_keys_decoded;
   } else if (name.equals("os_version") ||

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda marked 2 inline comments as done.
jasonmolenda added inline comments.



Comment at: 
lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py:39
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+).decode("utf-8")

bulbazord wrote:
> I know this is already done in a few places but we should really use 
> `llvm-dwarfdump` so this can be a more portable test...
Yeah Jonas made that point last week about another test I was getting the uuid 
from a file.  I don't know how to refer to something in the build products 
directory in an API test like this, is that what you meant?  It's not installed 
on a standard macOS system in any path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157659

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

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda updated this revision to Diff 549507.
jasonmolenda added a comment.

Incorporate Alex's suggestions, not sure about using llvm-dwarfdump yet tho.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157659

Files:
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/target-arch-from-module/Makefile
  lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
  lldb/test/API/python_api/target-arch-from-module/main.c

Index: lldb/test/API/python_api/target-arch-from-module/main.c
===
--- /dev/null
+++ lldb/test/API/python_api/target-arch-from-module/main.c
@@ -0,0 +1,5 @@
+#include 
+int main() {
+  puts("HI i am a program");
+  return 0;
+}
Index: lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
===
--- /dev/null
+++ lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
@@ -0,0 +1,107 @@
+"""
+An SBTarget with no arch, call AddModule, SBTarget's arch should be set.
+"""
+
+import os
+import subprocess
+import re
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetArchFromModule(TestBase):
+@skipIf(
+debug_info=no_match(["dsym"]),
+bugnumber="This test is looking explicitly for a dSYM",
+)
+@skipUnlessDarwin
+@skipIfRemote
+def test_target_arch_init(self):
+self.build()
+aout_exe = self.getBuildArtifact("a.out")
+aout_dsym = self.getBuildArtifact("a.out.dSYM")
+hidden_dir = self.getBuildArtifact("hide.noindex")
+hidden_aout_exe = self.getBuildArtifact("hide.noindex/a.out")
+hidden_aout_dsym = self.getBuildArtifact("hide.noindex/a.out.dSYM")
+dsym_for_uuid = self.getBuildArtifact("dsym-for-uuid.sh")
+
+# We can hook in our dsym-for-uuid shell script to lldb with
+# this env var instead of requiring a defaults write.
+os.environ["LLDB_APPLE_DSYMFORUUID_EXECUTABLE"] = dsym_for_uuid
+self.addTearDownHook(
+lambda: os.environ.pop("LLDB_APPLE_DSYMFORUUID_EXECUTABLE", None)
+)
+
+dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+).decode("utf-8")
+aout_uuid = None
+for line in dwarfdump_cmd_output.splitlines():
+match = dwarfdump_uuid_regex.search(line)
+if match:
+aout_uuid = match.group(1)
+self.assertNotEqual(aout_uuid, None, "Could not get uuid of built a.out")
+
+###  Create our dsym-for-uuid shell script which returns self.hidden_aout_exe.
+shell_cmds = [
+"#! /bin/sh",
+"# the last argument is the uuid",
+"while [ $# -gt 1 ]",
+"do",
+"  shift",
+"done",
+"ret=0",
+'echo ""',
+'echo "http://www.apple.com/DTDs/PropertyList-1.0.dtd\\";>"',
+'echo ""',
+"",
+'if [ "$1" = "%s" ]' % aout_uuid,
+"then",
+"  uuid=%s" % aout_uuid,
+"  bin=%s" % hidden_aout_exe,
+"  dsym=%s.dSYM/Contents/Resources/DWARF/%s"
+% (hidden_aout_exe, os.path.basename(hidden_aout_exe)),
+"fi",
+'echo "  "',
+'echo "$1"',
+'echo ""',
+'if [ -z "$uuid" -o -z "$bin" -o ! -f "$bin" ]',
+"then",
+'  echo "  DBGError"',
+'  echo "  not found by $0"',
+'  echo ""',
+'  echo "  "',
+'  echo ""',
+"  exit 0",
+"fi",
+"",
+'echo "DBGDSYMPath$dsym"',
+'echo "DBGSymbolRichExecutable$bin"',
+'echo ""',
+"exit $ret",
+]
+
+with open(dsym_for_uuid, "w") as writer:
+for l in shell_cmds:
+writer.write(l + "\n")
+
+os.chmod(dsym_for_uuid, 0o755)
+
+# Move the main binary and its dSYM into the hide.noindex
+# directory.  Now the only way lldb can find them is with
+# the LLDB_APPLE_DSYMFORUUID_EXECUTABLE shell script -
+# so we're testing that this dSYM discovery method works.
+lldbutil.mkdir_p(hidden_dir)
+os.rename(aout_exe, hidden_aout_exe)
+os.rename(aout_dsym, hidden_aout_dsym)
+
+target = self.dbg.CreateTarget("")
+self.assertTrue(target.IsValid())
+self.expect("target list", matching=False, substrs=["arch="])
+
+m = target.AddModule(None, None, aout_uuid)
+self.assertTrue(m.IsValid())
+self.expect(

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added inline comments.



Comment at: 
lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py:39
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+).decode("utf-8")

jasonmolenda wrote:
> bulbazord wrote:
> > I know this is already done in a few places but we should really use 
> > `llvm-dwarfdump` so this can be a more portable test...
> Yeah Jonas made that point last week about another test I was getting the 
> uuid from a file.  I don't know how to refer to something in the build 
> products directory in an API test like this, is that what you meant?  It's 
> not installed on a standard macOS system in any path.
Yeah I was referring to the build product `llvm-dwarfdump`. I did a quick 
search through the test suite but it doesn't look like we do this anywhere 
right now, so we'll need to figure out how to do it first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157659

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

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord accepted this revision.
bulbazord 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/D157659/new/

https://reviews.llvm.org/D157659

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

[Lldb-commits] [PATCH] D157667: Define qHostInfo and Mach-O LC_NOTE "addrable bits" methods to describe high and low memory addressing bits

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

LGTM. You may want to wait for Jonas to take a look before landing though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157667

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

[Lldb-commits] [PATCH] D157756: Support corefiles that put the uuid of their binary in a fixed address in low memory; load that binary

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

There are some Mach-O corefiles that store the UUID of the main binary at a 
list of fixed addresses in low memory, prefixed with the characters 'uuid' 16 
bytes before the 16-byte uuid itself.  This patch adds check in ProcessMachCore 
for this situation if we haven't found more authoritative metadata about the 
binary to load, tries to find the binary for that UUID and loads it in to the 
target with a slide of 0.  It's uncommon, but it would be nice if it was 
handled automatically and it's easy to drop in to ProcessMachCore at this point 
given the work I've been doing recently.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157756

Files:
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.h

Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
@@ -87,6 +87,8 @@
 private:
   void CreateMemoryRegions();
 
+  bool LoadBinaryViaLowmemUUID();
+
   /// \return
   ///   True if any metadata were found indicating the binary that should
   ///   be loaded, regardless of whether the specified binary could be found.
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
+#include "lldb/Utility/UUID.h"
 
 #include "ProcessMachCore.h"
 #include "Plugins/Process/Utility/StopInfoMachException.h"
@@ -223,6 +224,65 @@
   }
 }
 
+// Some corefiles have a UUID stored in a low memory
+// address.  We inspect a set list of addresses for
+// the characters 'uuid' and 16 bytes later there will
+// be a uuid_t UUID.  If we can find a binary that
+// matches the UUID, it is loaded with no slide in the target.
+bool ProcessMachCore::LoadBinaryViaLowmemUUID() {
+  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
+  ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
+
+  uint64_t lowmem_uuid_addresses[] = {0x2000204, 0x1000204, 0x120, 0x4204,
+  0x1204,0x1020,0x4020,0xc00,
+  0xC0,  0};
+
+  for (uint64_t addr : lowmem_uuid_addresses) {
+const VMRangeToFileOffset::Entry *core_memory_entry =
+m_core_aranges.FindEntryThatContains(addr);
+if (core_memory_entry) {
+  const addr_t offset = addr - core_memory_entry->GetRangeBase();
+  const addr_t bytes_left = core_memory_entry->GetRangeEnd() - addr;
+  if (bytes_left > 20) {
+char strbuf[4];
+if (core_objfile->CopyData(
+core_memory_entry->data.GetRangeBase() + offset, 4, &strbuf) &&
+strncmp("uuid", (char *)&strbuf, 4) == 0) {
+  uuid_t uuid_bytes;
+  if (core_objfile->CopyData(core_memory_entry->data.GetRangeBase() +
+ offset + 16,
+ sizeof(uuid_t), uuid_bytes)) {
+UUID uuid(uuid_bytes, sizeof(uuid_t));
+if (uuid.IsValid()) {
+  LLDB_LOGF(log,
+"ProcessMachCore::LoadBinaryViaLowmemUUID: found "
+"binary uuid %s at low memory address 0x%" PRIx64,
+uuid.GetAsString().c_str(), addr);
+  // We have no address specified, only a UUID.  Load it at the file
+  // address.
+  const bool value_is_offset = true;
+  const bool force_symbol_search = true;
+  const bool notify = true;
+  const bool set_address_in_target = true;
+  const bool allow_memory_image_last_resort = false;
+  if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
+  this, llvm::StringRef(), uuid, 0, value_is_offset,
+  force_symbol_search, notify, set_address_in_target,
+  allow_memory_image_last_resort)) {
+m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
+  }
+  // We found metadata saying which binary should be loaded; don't
+  // try an exhaustive search.
+  return true;
+}
+  }
+}
+  }
+}
+  }
+  return false;
+}
+
 bool ProcessMachCore::LoadBinariesViaMetadata() {
   Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::P

[Lldb-commits] [PATCH] D157756: Support corefiles that put the uuid of their binary in a fixed address in low memory; load that binary

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda updated this revision to Diff 549516.
jasonmolenda added a comment.

Fix size check when making sure there's enough data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157756

Files:
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.h

Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
@@ -87,6 +87,8 @@
 private:
   void CreateMemoryRegions();
 
+  bool LoadBinaryViaLowmemUUID();
+
   /// \return
   ///   True if any metadata were found indicating the binary that should
   ///   be loaded, regardless of whether the specified binary could be found.
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
+#include "lldb/Utility/UUID.h"
 
 #include "ProcessMachCore.h"
 #include "Plugins/Process/Utility/StopInfoMachException.h"
@@ -223,6 +224,66 @@
   }
 }
 
+// Some corefiles have a UUID stored in a low memory
+// address.  We inspect a set list of addresses for
+// the characters 'uuid' and 16 bytes later there will
+// be a uuid_t UUID.  If we can find a binary that
+// matches the UUID, it is loaded with no slide in the target.
+bool ProcessMachCore::LoadBinaryViaLowmemUUID() {
+  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
+  ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
+
+  uint64_t lowmem_uuid_addresses[] = {0x2000204, 0x1000204, 0x120, 0x4204,
+  0x1204,0x1020,0x4020,0xc00,
+  0xC0,  0};
+
+  for (uint64_t addr : lowmem_uuid_addresses) {
+const VMRangeToFileOffset::Entry *core_memory_entry =
+m_core_aranges.FindEntryThatContains(addr);
+if (core_memory_entry) {
+  const addr_t offset = addr - core_memory_entry->GetRangeBase();
+  const addr_t bytes_left = core_memory_entry->GetRangeEnd() - addr;
+  // (4-bytes 'uuid' + 12 bytes pad for align + 16 bytes uuid_t) == 32 bytes
+  if (bytes_left >= 32) {
+char strbuf[4];
+if (core_objfile->CopyData(
+core_memory_entry->data.GetRangeBase() + offset, 4, &strbuf) &&
+strncmp("uuid", (char *)&strbuf, 4) == 0) {
+  uuid_t uuid_bytes;
+  if (core_objfile->CopyData(core_memory_entry->data.GetRangeBase() +
+ offset + 16,
+ sizeof(uuid_t), uuid_bytes)) {
+UUID uuid(uuid_bytes, sizeof(uuid_t));
+if (uuid.IsValid()) {
+  LLDB_LOGF(log,
+"ProcessMachCore::LoadBinaryViaLowmemUUID: found "
+"binary uuid %s at low memory address 0x%" PRIx64,
+uuid.GetAsString().c_str(), addr);
+  // We have no address specified, only a UUID.  Load it at the file
+  // address.
+  const bool value_is_offset = true;
+  const bool force_symbol_search = true;
+  const bool notify = true;
+  const bool set_address_in_target = true;
+  const bool allow_memory_image_last_resort = false;
+  if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
+  this, llvm::StringRef(), uuid, 0, value_is_offset,
+  force_symbol_search, notify, set_address_in_target,
+  allow_memory_image_last_resort)) {
+m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
+  }
+  // We found metadata saying which binary should be loaded; don't
+  // try an exhaustive search.
+  return true;
+}
+  }
+}
+  }
+}
+  }
+  return false;
+}
+
 bool ProcessMachCore::LoadBinariesViaMetadata() {
   Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
   ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
@@ -338,6 +399,9 @@
 m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
   }
 
+  if (!found_binary_spec_in_metadata && LoadBinaryViaLowmemUUID())
+found_binary_spec_in_metadata = true;
+
   // LoadCoreFileImges may have set the dynamic loader, e.g. in
   // PlatformDarwinKernel::LoadPlatformBinaryAndSetup().
   // If we now have a dynamic loader, save its name so we don't 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
h

[Lldb-commits] [lldb] ebf2490 - [lldb] SBTarget::AddModule do all searches by UUID, set Target arch

2023-08-11 Thread Jason Molenda via lldb-commits 

Author: Jason Molenda
Date: 2023-08-11T14:20:38-07:00
New Revision: ebf249066ac5c7917064eb56a9e51c21000cdf93

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

LOG: [lldb] SBTarget::AddModule do all searches by UUID, set Target arch

Make SBTarget::AddModule possibly call out to an external program to
find the binary by UUID if it can't be found more easily, the same
way `target modules add -u ...` works from the commandline.

If the Target does not have an architecture set yet, use the
Module's Arch to initialize it.  Allows an API writer to create
a target with no arch, and inherit it from the first binary they
load with AddModules.

Differential Revision: https://reviews.llvm.org/D157659
rdar://113657555

Added: 
lldb/test/API/python_api/target-arch-from-module/Makefile
lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
lldb/test/API/python_api/target-arch-from-module/main.c

Modified: 
lldb/source/API/SBTarget.cpp

Removed: 




diff  --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 3fef7428abe24b..e38edf8e44652e 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "lldb/API/SBTarget.h"
+#include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Utility/Instrumentation.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/lldb-public.h"
@@ -1477,28 +1478,29 @@ lldb::SBModule SBTarget::AddModule(const char *path, 
const char *triple,
const char *uuid_cstr, const char *symfile) 
{
   LLDB_INSTRUMENT_VA(this, path, triple, uuid_cstr, symfile);
 
-  lldb::SBModule sb_module;
   TargetSP target_sp(GetSP());
-  if (target_sp) {
-ModuleSpec module_spec;
-if (path)
-  module_spec.GetFileSpec().SetFile(path, FileSpec::Style::native);
+  if (!target_sp)
+return {};
 
-if (uuid_cstr)
-  module_spec.GetUUID().SetFromStringRef(uuid_cstr);
+  ModuleSpec module_spec;
+  if (path)
+module_spec.GetFileSpec().SetFile(path, FileSpec::Style::native);
 
-if (triple)
-  module_spec.GetArchitecture() = Platform::GetAugmentedArchSpec(
-  target_sp->GetPlatform().get(), triple);
-else
-  module_spec.GetArchitecture() = target_sp->GetArchitecture();
+  if (uuid_cstr)
+module_spec.GetUUID().SetFromStringRef(uuid_cstr);
 
-if (symfile)
-  module_spec.GetSymbolFileSpec().SetFile(symfile, 
FileSpec::Style::native);
+  if (triple)
+module_spec.GetArchitecture() =
+Platform::GetAugmentedArchSpec(target_sp->GetPlatform().get(), triple);
+  else
+module_spec.GetArchitecture() = target_sp->GetArchitecture();
 
-sb_module.SetSP(target_sp->GetOrCreateModule(module_spec, true /* notify 
*/));
-  }
-  return sb_module;
+  if (symfile)
+module_spec.GetSymbolFileSpec().SetFile(symfile, FileSpec::Style::native);
+
+  SBModuleSpec sb_modulespec(module_spec);
+
+  return AddModule(sb_modulespec);
 }
 
 lldb::SBModule SBTarget::AddModule(const SBModuleSpec &module_spec) {
@@ -1506,9 +1508,26 @@ lldb::SBModule SBTarget::AddModule(const SBModuleSpec 
&module_spec) {
 
   lldb::SBModule sb_module;
   TargetSP target_sp(GetSP());
-  if (target_sp)
+  if (target_sp) {
 sb_module.SetSP(target_sp->GetOrCreateModule(*module_spec.m_opaque_up,
  true /* notify */));
+if (!sb_module.IsValid() && module_spec.m_opaque_up->GetUUID().IsValid()) {
+  Status error;
+  if (Symbols::DownloadObjectAndSymbolFile(*module_spec.m_opaque_up, error,
+   /* force_lookup */ true)) {
+if (FileSystem::Instance().Exists(
+module_spec.m_opaque_up->GetFileSpec())) {
+  
sb_module.SetSP(target_sp->GetOrCreateModule(*module_spec.m_opaque_up,
+   true /* notify */));
+}
+  }
+}
+  }
+  // If the target hasn't initialized any architecture yet, use the
+  // binary's architecture.
+  if (sb_module.IsValid() && !target_sp->GetArchitecture().IsValid() &&
+  sb_module.GetSP()->GetArchitecture().IsValid())
+target_sp->SetArchitecture(sb_module.GetSP()->GetArchitecture());
   return sb_module;
 }
 

diff  --git a/lldb/test/API/python_api/target-arch-from-module/Makefile 
b/lldb/test/API/python_api/target-arch-from-module/Makefile
new file mode 100644
index 00..10495940055b63
--- /dev/null
+++ b/lldb/test/API/python_api/target-arch-from-module/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules

diff  --git 
a/lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py 
b/lldb/test/API/pytho

[Lldb-commits] [PATCH] D157659: Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

2023-08-11 Thread Jason Molenda via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGebf249066ac5: [lldb] SBTarget::AddModule do all searches by 
UUID, set Target arch (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157659

Files:
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/target-arch-from-module/Makefile
  lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
  lldb/test/API/python_api/target-arch-from-module/main.c

Index: lldb/test/API/python_api/target-arch-from-module/main.c
===
--- /dev/null
+++ lldb/test/API/python_api/target-arch-from-module/main.c
@@ -0,0 +1,5 @@
+#include 
+int main() {
+  puts("HI i am a program");
+  return 0;
+}
Index: lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
===
--- /dev/null
+++ lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py
@@ -0,0 +1,107 @@
+"""
+An SBTarget with no arch, call AddModule, SBTarget's arch should be set.
+"""
+
+import os
+import subprocess
+import re
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetArchFromModule(TestBase):
+@skipIf(
+debug_info=no_match(["dsym"]),
+bugnumber="This test is looking explicitly for a dSYM",
+)
+@skipUnlessDarwin
+@skipIfRemote
+def test_target_arch_init(self):
+self.build()
+aout_exe = self.getBuildArtifact("a.out")
+aout_dsym = self.getBuildArtifact("a.out.dSYM")
+hidden_dir = self.getBuildArtifact("hide.noindex")
+hidden_aout_exe = self.getBuildArtifact("hide.noindex/a.out")
+hidden_aout_dsym = self.getBuildArtifact("hide.noindex/a.out.dSYM")
+dsym_for_uuid = self.getBuildArtifact("dsym-for-uuid.sh")
+
+# We can hook in our dsym-for-uuid shell script to lldb with
+# this env var instead of requiring a defaults write.
+os.environ["LLDB_APPLE_DSYMFORUUID_EXECUTABLE"] = dsym_for_uuid
+self.addTearDownHook(
+lambda: os.environ.pop("LLDB_APPLE_DSYMFORUUID_EXECUTABLE", None)
+)
+
+dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+).decode("utf-8")
+aout_uuid = None
+for line in dwarfdump_cmd_output.splitlines():
+match = dwarfdump_uuid_regex.search(line)
+if match:
+aout_uuid = match.group(1)
+self.assertNotEqual(aout_uuid, None, "Could not get uuid of built a.out")
+
+###  Create our dsym-for-uuid shell script which returns self.hidden_aout_exe.
+shell_cmds = [
+"#! /bin/sh",
+"# the last argument is the uuid",
+"while [ $# -gt 1 ]",
+"do",
+"  shift",
+"done",
+"ret=0",
+'echo ""',
+'echo "http://www.apple.com/DTDs/PropertyList-1.0.dtd\\";>"',
+'echo ""',
+"",
+'if [ "$1" = "%s" ]' % aout_uuid,
+"then",
+"  uuid=%s" % aout_uuid,
+"  bin=%s" % hidden_aout_exe,
+"  dsym=%s.dSYM/Contents/Resources/DWARF/%s"
+% (hidden_aout_exe, os.path.basename(hidden_aout_exe)),
+"fi",
+'echo "  "',
+'echo "$1"',
+'echo ""',
+'if [ -z "$uuid" -o -z "$bin" -o ! -f "$bin" ]',
+"then",
+'  echo "  DBGError"',
+'  echo "  not found by $0"',
+'  echo ""',
+'  echo "  "',
+'  echo ""',
+"  exit 0",
+"fi",
+"",
+'echo "DBGDSYMPath$dsym"',
+'echo "DBGSymbolRichExecutable$bin"',
+'echo ""',
+"exit $ret",
+]
+
+with open(dsym_for_uuid, "w") as writer:
+for l in shell_cmds:
+writer.write(l + "\n")
+
+os.chmod(dsym_for_uuid, 0o755)
+
+# Move the main binary and its dSYM into the hide.noindex
+# directory.  Now the only way lldb can find them is with
+# the LLDB_APPLE_DSYMFORUUID_EXECUTABLE shell script -
+# so we're testing that this dSYM discovery method works.
+lldbutil.mkdir_p(hidden_dir)
+os.rename(aout_exe, hidden_aout_exe)
+os.rename(aout_dsym, hidden_aout_dsym)
+
+target = self.dbg.CreateTarget("")
+self.assertTrue(target.IsValid())
+self.expect("target list", matching=False, substrs=["arch="])
+
+m = target.AddModule(None, None, aout_uuid)
+self.a

[Lldb-commits] [PATCH] D157759: [lldb] Remove use of __future__ in python

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, mib, kastiglione, DavidSpickett.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

These were useful primarily for the Python 2 to 3 transition. Python 2
is no longer supported so these are no longer necessary.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157759

Files:
  lldb/docs/conf.py
  lldb/packages/Python/lldbsuite/support/gmodules.py
  lldb/packages/Python/lldbsuite/test/__init__.py
  lldb/packages/Python/lldbsuite/test/bench.py
  lldb/packages/Python/lldbsuite/test/configuration.py
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/dotest_args.py
  lldb/packages/Python/lldbsuite/test/lldb_pylint_helper.py
  lldb/packages/Python/lldbsuite/test/lldbbench.py
  lldb/packages/Python/lldbsuite/test/lldbinline.py
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/packages/Python/lldbsuite/test/lldbplatform.py
  lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/packages/Python/lldbsuite/test/test_categories.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
  lldb/scripts/android/host_art_bt.py
  lldb/third_party/Python/module/progress/progress.py
  lldb/third_party/Python/module/unittest2/unittest2/test/test_result.py
  lldb/third_party/Python/module/unittest2/unittest2/test/test_unittest2_with.py
  lldb/tools/intel-features/intel-mpx/test/TestMPXTable.py
  lldb/utils/lui/lldbutil.py

Index: lldb/utils/lui/lldbutil.py
===
--- lldb/utils/lui/lldbutil.py
+++ lldb/utils/lui/lldbutil.py
@@ -12,8 +12,6 @@
 They can also be useful for general purpose lldb scripting.
 """
 
-from __future__ import print_function
-
 import lldb
 import os
 import sys
Index: lldb/tools/intel-features/intel-mpx/test/TestMPXTable.py
===
--- lldb/tools/intel-features/intel-mpx/test/TestMPXTable.py
+++ lldb/tools/intel-features/intel-mpx/test/TestMPXTable.py
@@ -2,9 +2,6 @@
 Test mpx-table command.
 """
 
-from __future__ import print_function
-
-
 import os
 import time
 import re
Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_unittest2_with.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_unittest2_with.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_unittest2_with.py
@@ -1,5 +1,3 @@
-from __future__ import with_statement
-
 import unittest2
 from unittest2.test.support import OldTestResult, catch_warnings
 
Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_result.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_result.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_result.py
@@ -1,5 +1,3 @@
-from __future__ import print_function
-
 import sys
 import textwrap
 from StringIO import StringIO
Index: lldb/third_party/Python/module/progress/progress.py
===
--- lldb/third_party/Python/module/progress/progress.py
+++ lldb/third_party/Python/module/progress/progress.py
@@ -1,7 +1,5 @@
 #!/usr/bin/env python
 
-from __future__ import print_function
-
 import use_lldb_suite
 
 import sys
Index: lldb/scripts/android/host_art_bt.py
===
--- lldb/scripts/android/host_art_bt.py
+++ lldb/scripts/android/host_art_bt.py
@@ -5,8 +5,6 @@
 #   'command script import host_art_bt.py'
 #   'host_art_bt'
 
-from __future__ import print_function
-
 import sys
 import re
 
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
@@ -1,8 +1,6 @@
 """Module for supporting unit testing of the lldb-server debug monitor exe.
 """
 
-from __future__ import division, print_function
-
 import binascii
 import os
 import os.path
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
@@ -2,9 +2,6 @@
 Base class for gdb-remote test cases.
 """
 
-from __future__ import

[Lldb-commits] [PATCH] D157760: [lldb] Properly protect the Communication class with reader/writer lock

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 created this revision.
augusto2112 added reviewers: JDevlieghere, bulbazord.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch picks up where https://reviews.llvm.org/D157159 left of, but
allows for concurrent reads/writes, but protects setting up and tearing
down the underlying Connection object.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157760

Files:
  lldb/include/lldb/Core/Communication.h
  lldb/include/lldb/Core/ThreadedCommunication.h
  lldb/source/Core/Communication.cpp
  lldb/source/Core/ThreadedCommunication.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

Index: lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
===
--- lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -557,7 +557,6 @@
 }
   }
   StopAsyncThread();
-  m_comm.Clear();
 
   SetPrivateState(eStateDetached);
   ResumePrivateStateThread();
Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -61,11 +61,6 @@
this, GetBroadcasterName());
 }
 
-void ThreadedCommunication::Clear() {
-  SetReadThreadBytesReceivedCallback(nullptr, nullptr);
-  StopReadThread(nullptr);
-  Communication::Clear();
-}
 
 ConnectionStatus ThreadedCommunication::Disconnect(Status *error_ptr) {
   assert((!m_read_thread_enabled || m_read_thread_did_exit) &&
@@ -77,6 +72,7 @@
const Timeout &timeout,
ConnectionStatus &status,
Status *error_ptr) {
+  std::shared_lock guard(m_connection_mutex);
   Log *log = GetLog(LLDBLog::Communication);
   LLDB_LOG(
   log,
@@ -152,7 +148,7 @@
 
   // We aren't using a read thread, just read the data synchronously in this
   // thread.
-  return Communication::Read(dst, dst_len, timeout, status, error_ptr);
+  return Communication::ReadUnlocked(dst, dst_len, timeout, status, error_ptr);
 }
 
 bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
@@ -273,46 +269,50 @@
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
   bool disconnect = false;
-  while (!done && m_read_thread_enabled) {
-size_t bytes_read = ReadFromConnection(
-buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
-  AppendBytesToCache(buf, bytes_read, true, status);
-
-switch (status) {
-case eConnectionStatusSuccess:
-  break;
-
-case eConnectionStatusEndOfFile:
-  done = true;
-  disconnect = GetCloseOnEOF();
-  break;
-case eConnectionStatusError: // Check GetError() for details
-  if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
-// EIO on a pipe is usually caused by remote shutdown
+
+  {
+std::shared_lock guard(m_connection_mutex);
+while (!done && m_read_thread_enabled) {
+  size_t bytes_read = ReadUnlocked(buf, sizeof(buf),
+   std::chrono::seconds(5), status, &error);
+  if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
+AppendBytesToCache(buf, bytes_read, true, status);
+
+  switch (status) {
+  case eConnectionStatusSuccess:
+break;
+
+  case eConnectionStatusEndOfFile:
+done = true;
 disconnect = GetCloseOnEOF();
+break;
+  case eConnectionStatusError: // Check GetError() for details
+if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
+  // EIO on a pipe is usually caused by remote shutdown
+  disconnect = GetCloseOnEOF();
+  done = true;
+}
+if (error.Fail())
+  LLDB_LOG(log, "error: {0}, status = {1}", error,
+   ThreadedCommunication::ConnectionStatusAsString(status));
+break;
+  case eConnectionStatusInterrupted: // Synchronization signal from
+ // SynchronizeWithReadThread()
+// The connection returns eConnectionStatusInterrupted only when there
+// is no input pending to be read, so we can signal that.
+BroadcastEvent(eBroadcastBitNoMorePendingInput);
+break;
+  case eConnectionStatusNoConnection:   // No connection
+  case eConnectionStatusLostConnection: // Lost connection while connected
+// to a valid connection
 done = true;
+[[fallthrough]];
+  case eConnectionStatusTimedOut: // Request timed out
+if (error.Fail())
+  LLDB_LOG(log, "error: {0}, status = {1}", error,
+   ThreadedCommuni

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

2023-08-11 Thread walter erquinigo via Phabricator via lldb-commits 
wallace created this revision.
wallace added a reviewer: bulbazord.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

https://github.com/llvm/llvm-project/commit/59237bb52c9483fce395428bfab5996eabe54ed0
 changed the behavior of the `SetTimeout` and `GetTimeout` methods of 
`EvaluateExpressionOptions`, which broke the Mojo REPL and related services 
(https://docs.modular.com/mojo/) because it relies on having infinite timeouts. 
That's a necessity because developers often use the REPL for executing 
extremely long-running numeric jobs. Having said that, 
`EvaluateExpressionOptions` shouldn't be that opinionated on this matter 
anyway. Instead, it should be the responsibility of the evaluator to define 
which timeout to use for each specific case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157764

Files:
  lldb/include/lldb/Target/Target.h


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 updated this revision to Diff 549539.
augusto2112 added a comment.

Update to two shared mutexes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

Files:
  lldb/include/lldb/Host/posix/PipePosix.h
  lldb/source/Host/posix/PipePosix.cpp

Index: lldb/source/Host/posix/PipePosix.cpp
===
--- lldb/source/Host/posix/PipePosix.cpp
+++ lldb/source/Host/posix/PipePosix.cpp
@@ -65,16 +65,21 @@
 pipe_posix.ReleaseWriteFileDescriptor()} {}
 
 PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
+  std::scoped_lock guard(m_read_mutex, m_write_mutex, pipe_posix.m_read_mutex,
+ pipe_posix.m_write_mutex);
+
   PipeBase::operator=(std::move(pipe_posix));
-  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
-  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();
+  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptorUnlocked();
   return *this;
 }
 
 PipePosix::~PipePosix() { Close(); }
 
 Status PipePosix::CreateNew(bool child_processes_inherit) {
-  if (CanRead() || CanWrite())
+  // Lock both mutexes in exclusive mode.
+  std::scoped_lock guard(m_read_mutex, m_write_mutex);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status(EINVAL, eErrorTypePOSIX);
 
   Status error;
@@ -87,7 +92,7 @@
 if (!child_processes_inherit) {
   if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
 error.SetErrorToErrno();
-Close();
+CloseUnlocked();
 return error;
   }
 }
@@ -103,7 +108,11 @@
 }
 
 Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  // Lock both mutexes in shared mode.
+  std::shared_lock read_guard(m_read_mutex, std::defer_lock);
+  std::shared_lock write_guard(m_write_mutex, std::defer_lock);
+  std::lock(read_guard, write_guard);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   Status error;
@@ -140,7 +149,13 @@
 
 Status PipePosix::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  // Lock read mutex in exclusive mode and write mutex in shared mode, since we
+  // only write the read file descriptor.
+  std::unique_lock read_guard(m_read_mutex, std::defer_lock);
+  std::shared_lock write_guard(m_write_mutex, std::defer_lock);
+  std::lock(read_guard, write_guard);
+
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_RDONLY | O_NONBLOCK;
@@ -161,7 +176,8 @@
 PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
bool child_process_inherit,
const std::chrono::microseconds &timeout) {
-  if (CanRead() || CanWrite())
+  std::unique_lock guard(m_write_mutex);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_WRONLY | O_NONBLOCK;
@@ -171,7 +187,7 @@
   using namespace std::chrono;
   const auto finish_time = Now() + timeout;
 
-  while (!CanWrite()) {
+  while (!CanWriteUnlocked()) {
 if (timeout != microseconds::zero()) {
   const auto dur = duration_cast(finish_time - Now()).count();
   if (dur <= 0)
@@ -196,25 +212,54 @@
   return Status();
 }
 
-int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; }
+int PipePosix::GetReadFileDescriptor() const {
+  std::shared_lock guad(m_read_mutex);
+  return GetReadFileDescriptorUnlocked();
+}
+
+int PipePosix::GetReadFileDescriptorUnlocked() const {
+  return m_fds[READ];
+}
 
-int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; }
+int PipePosix::GetWriteFileDescriptor() const {
+  std::shared_lock guard(m_write_mutex);
+  return GetWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::GetWriteFileDescriptorUnlocked() const {
+  return m_fds[WRITE];
+}
 
 int PipePosix::ReleaseReadFileDescriptor() {
+  std::unique_lock guard(m_read_mutex);
+  return ReleaseReadFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseReadFileDescriptorUnlocked() {
   const int fd = m_fds[READ];
   m_fds[READ] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 int PipePosix::ReleaseWriteFileDescriptor() {
+  std::unique_lock guard(m_write_mutex);
+  return ReleaseWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseWriteFileDescriptorUnlocked() {
   const int fd = m_fds[WRITE];
   m_fds[WRITE] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 void PipePosix::Close() {
-  CloseReadFileDescriptor();
-  CloseWriteFileDescriptor();
+  std::scoped_lock guard(m_read_mutex, m_write_mutex);
+  CloseUnlocked();
+}
+
+void PipePosix::CloseUnlocked() {
+  CloseReadFileDescriptorUnlocked();
+  CloseWriteFileDescriptorUnlocked();
 }
 
 St

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added a comment.

@JDevlieghere I updated this to two shared mutexes because I'm assuming you can 
have more than one concurrent read and more than one concurrent write. If this 
is wrong I can go back to two regular mutexes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

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

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D157654#4581578 , @augusto2112 
wrote:

> @JDevlieghere I updated this to two shared mutexes because I'm assuming you 
> can have more than one concurrent read and more than one concurrent write. If 
> this is wrong I can go back to two regular mutexes.

According to POSIX [1] that would be undefined:

> The behavior of multiple concurrent reads on the same pipe, FIFO, or terminal 
> device is unspecified.

A regular mutex should be fine.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

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

[Lldb-commits] [PATCH] D157759: [lldb] Remove use of __future__ in python

2023-08-11 Thread 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! Although we should make sure no one is still using Python 2 downstream


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157759

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

[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix

2023-08-11 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 updated this revision to Diff 549548.
augusto2112 added a comment.

Changed to regular mutex


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

Files:
  lldb/include/lldb/Host/posix/PipePosix.h
  lldb/source/Host/posix/PipePosix.cpp

Index: lldb/source/Host/posix/PipePosix.cpp
===
--- lldb/source/Host/posix/PipePosix.cpp
+++ lldb/source/Host/posix/PipePosix.cpp
@@ -65,16 +65,20 @@
 pipe_posix.ReleaseWriteFileDescriptor()} {}
 
 PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
+  std::scoped_lock guard(m_read_mutex, m_write_mutex, pipe_posix.m_read_mutex,
+ pipe_posix.m_write_mutex);
+
   PipeBase::operator=(std::move(pipe_posix));
-  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
-  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();
+  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptorUnlocked();
   return *this;
 }
 
 PipePosix::~PipePosix() { Close(); }
 
 Status PipePosix::CreateNew(bool child_processes_inherit) {
-  if (CanRead() || CanWrite())
+  std::scoped_lock guard(m_read_mutex, m_write_mutex);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status(EINVAL, eErrorTypePOSIX);
 
   Status error;
@@ -87,7 +91,7 @@
 if (!child_processes_inherit) {
   if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
 error.SetErrorToErrno();
-Close();
+CloseUnlocked();
 return error;
   }
 }
@@ -103,7 +107,8 @@
 }
 
 Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  std::scoped_lock (m_read_mutex, m_write_mutex);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   Status error;
@@ -140,7 +145,9 @@
 
 Status PipePosix::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  std::scoped_lock (m_read_mutex, m_write_mutex);
+
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_RDONLY | O_NONBLOCK;
@@ -161,7 +168,8 @@
 PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
bool child_process_inherit,
const std::chrono::microseconds &timeout) {
-  if (CanRead() || CanWrite())
+  std::lock_guard guard(m_write_mutex);
+  if (CanReadUnlocked() || CanWriteUnlocked())
 return Status("Pipe is already opened");
 
   int flags = O_WRONLY | O_NONBLOCK;
@@ -171,7 +179,7 @@
   using namespace std::chrono;
   const auto finish_time = Now() + timeout;
 
-  while (!CanWrite()) {
+  while (!CanWriteUnlocked()) {
 if (timeout != microseconds::zero()) {
   const auto dur = duration_cast(finish_time - Now()).count();
   if (dur <= 0)
@@ -196,25 +204,54 @@
   return Status();
 }
 
-int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; }
+int PipePosix::GetReadFileDescriptor() const {
+  std::lock_guard guard(m_read_mutex);
+  return GetReadFileDescriptorUnlocked();
+}
+
+int PipePosix::GetReadFileDescriptorUnlocked() const {
+  return m_fds[READ];
+}
 
-int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; }
+int PipePosix::GetWriteFileDescriptor() const {
+  std::lock_guard guard(m_write_mutex);
+  return GetWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::GetWriteFileDescriptorUnlocked() const {
+  return m_fds[WRITE];
+}
 
 int PipePosix::ReleaseReadFileDescriptor() {
+  std::lock_guard guard(m_read_mutex);
+  return ReleaseReadFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseReadFileDescriptorUnlocked() {
   const int fd = m_fds[READ];
   m_fds[READ] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 int PipePosix::ReleaseWriteFileDescriptor() {
+  std::lock_guard guard(m_write_mutex);
+  return ReleaseWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseWriteFileDescriptorUnlocked() {
   const int fd = m_fds[WRITE];
   m_fds[WRITE] = PipePosix::kInvalidDescriptor;
   return fd;
 }
 
 void PipePosix::Close() {
-  CloseReadFileDescriptor();
-  CloseWriteFileDescriptor();
+  std::scoped_lock guard(m_read_mutex, m_write_mutex);
+  CloseUnlocked();
+}
+
+void PipePosix::CloseUnlocked() {
+  CloseReadFileDescriptorUnlocked();
+  CloseWriteFileDescriptorUnlocked();
 }
 
 Status PipePosix::Delete(llvm::StringRef name) {
@@ -222,22 +259,41 @@
 }
 
 bool PipePosix::CanRead() const {
+  std::lock_guard guard(m_read_mutex);
+  return CanReadUnlocked();
+}
+
+bool PipePosix::CanReadUnlocked() const {
   return m_fds[READ] != PipePosix::kInvalidDescriptor;
 }
 
 bool PipePosix::CanWrite() const {
+  std::lock_guard guard(m_write_mutex);
+  return CanWriteUnlocked();
+}
+
+bool PipePosix::CanWriteUnlocked() const {
   ret

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

2023-08-11 Thread River Riddle via Phabricator via lldb-commits 
rriddle added a comment.

This makes sense to me, but again will defer to area owners for what the 
expectation is here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

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

[Lldb-commits] [PATCH] D157137: [lldb/crashlog] Skip images with empty path and 0 UUID from loading

2023-08-11 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib updated this revision to Diff 549560.
mib added a comment.

Address @bulbazord comments.


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

https://reviews.llvm.org/D157137

Files:
  lldb/examples/python/symbolication.py


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -396,6 +396,9 @@
 
 def add_module(self, target, obj_dir=None):
 """Add the Image described in this object to "target" and load the 
sections if "load" is True."""
+if not self.path and self.uuid == uuid.UUID(int=0):
+return "error: invalid image (%s) " % self.identifier
+
 if target:
 # Try and find using UUID only first so that paths need not match
 # up


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -396,6 +396,9 @@
 
 def add_module(self, target, obj_dir=None):
 """Add the Image described in this object to "target" and load the sections if "load" is True."""
+if not self.path and self.uuid == uuid.UUID(int=0):
+return "error: invalid image (%s) " % self.identifier
+
 if target:
 # Try and find using UUID only first so that paths need not match
 # up
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

2023-08-11 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added a comment.

Btw, the SBAPI expects the old behavior.

  // Set the timeout for the expression, 0 means wait forever.
  void SetTimeoutInMicroSeconds(uint32_t timeout = 0);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

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

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

2023-08-11 Thread Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits 
RamNalamothu added a comment.

Thank you @jasonmolenda.

If there are no further comments on llvm side, I will land this in a couple of 
days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156086

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

[Lldb-commits] [PATCH] D157137: [lldb/crashlog] Skip images with empty path and 0 UUID from loading

2023-08-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D157137

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