[Lldb-commits] [PATCH] D157361: [lldb] FIx data race in ThreadedCommunication

[Jonas Devlieghere via Phabricator via lldb-commits]

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

TSan reports the following race:

  Write of size 8 at 0x000107707ee8 by main thread:
#0 lldb_private::ThreadedCommunication::StartReadThread(...) 
ThreadedCommunication.cpp:175
#1 lldb_private::Process::SetSTDIOFileDescriptor(...) Process.cpp:4533
#2 lldb_private::Platform::DebugProcess(...) Platform.cpp:1121
#3 lldb_private::PlatformDarwin::DebugProcess(...) PlatformDarwin.cpp:711
#4 lldb_private::Target::Launch(...) Target.cpp:3235
#5 CommandObjectProcessLaunch::DoExecute(...) CommandObjectProcess.cpp:256
#6 lldb_private::CommandObjectParsed::Execute(...) CommandObject.cpp:751
#7 lldb_private::CommandInterpreter::HandleCommand(...) 
CommandInterpreter.cpp:2054
  
  Previous read of size 8 at 0x000107707ee8 by thread T5:
#0 lldb_private::HostThread::IsJoinable(...) const HostThread.cpp:30
#1 lldb_private::ThreadedCommunication::StopReadThread(...) 
ThreadedCommunication.cpp:192
#2 lldb_private::Process::ShouldBroadcastEvent(...) Process.cpp:3420
#3 lldb_private::Process::HandlePrivateEvent(...) Process.cpp:3728
#4 lldb_private::Process::RunPrivateStateThread(...) Process.cpp:3914
#5 
std::__1::__function::__funchttps://reviews.llvm.org/D157361

Files:
  lldb/include/lldb/Core/ThreadedCommunication.h
  lldb/source/Core/ThreadedCommunication.cpp


Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -155,6 +156,8 @@
 }
 
 bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (error_ptr)
 error_ptr->Clear();
 
@@ -189,6 +192,8 @@
 }
 
 bool ThreadedCommunication::StopReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (!m_read_thread.IsJoinable())
 return true;
 
@@ -199,13 +204,13 @@
 
   BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr);
 
-  // error = m_read_thread.Cancel();
-
   Status error = m_read_thread.Join(nullptr);
   return error.Success();
 }
 
 bool ThreadedCommunication::JoinReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (!m_read_thread.IsJoinable())
 return true;
 
Index: lldb/include/lldb/Core/ThreadedCommunication.h
===
--- lldb/include/lldb/Core/ThreadedCommunication.h
+++ lldb/include/lldb/Core/ThreadedCommunication.h
@@ -223,10 +223,22 @@
   }
 
 protected:
-  HostThread m_read_thread; ///< The read thread handle in case we need to
-/// cancel the thread.
+  /// The read thread handle in case we need to cancel the thread.
+  /// @{
+  HostThread m_read_thread;
+  std::mutex m_read_thread_mutex;
+  /// @}
+
+  /// Whether the read thread is enabled. This cannot be guarded by the read
+  /// thread mutex becuase it is used as the control variable to exit the read
+  /// thread.
   std::atomic m_read_thread_enabled;
+
+  /// Whether the read thread is enabled. Technically this could be guarded by
+  /// the read thread mutex but that needlessly complicates things to
+  /// check this variables momentary value.
   std::atomic m_read_thread_did_exit;
+
   std::string
   m_bytes; ///< A buffer to cache bytes read in the ReadThread function.
   std::recursive_mutex m_bytes_mutex;   ///< A mutex to protect multi-threaded


Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -155,6 +156,8 @@
 }
 
 bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (error_ptr)
 error_ptr->Clear();
 
@@ -189,6 +192,8 @@
 }
 
 bool ThreadedCommunication::StopReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (!m_read_thread.IsJoinable())
 return true;
 
@@ -199,13 +204,13 @@
 
   BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr);
 
-  // error = m_read_thread.Cancel();
-
   Status error = m_read_thread.Join(nullptr);
   return error.Success();
 }
 
 bool ThreadedCommunication::JoinReadThread(Status *error_ptr) {
+  std::lock_guard lock(m_read_thread_mutex);
+
   if (!m_read_thread.IsJoinable())
 return true;
 
Index: lldb/include/lldb/Core/ThreadedCommunication.h
===
--- lldb/include/lldb/Core/ThreadedCommunication.h
+++ lldb/include/lldb/Core/ThreadedCommunication.h
@@ -2

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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere abandoned this revision.
JDevlieghere added a comment.

Abandoning this in favor of D157347 


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

https://reviews.llvm.org/D157159

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

[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 548056.

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

https://reviews.llvm.org/D157347

Files:
  lldb/include/lldb/Host/File.h
  lldb/source/Host/common/File.cpp

Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -78,21 +78,20 @@
 }
 
 Expected File::GetOptionsFromMode(llvm::StringRef mode) {
-  OpenOptions opts =
-  llvm::StringSwitch(mode)
-  .Cases("r", "rb", eOpenOptionReadOnly)
-  .Cases("w", "wb", eOpenOptionWriteOnly)
-  .Cases("a", "ab",
- eOpenOptionWriteOnly | eOpenOptionAppend |
- eOpenOptionCanCreate)
-  .Cases("r+", "rb+", "r+b", eOpenOptionReadWrite)
-  .Cases("w+", "wb+", "w+b",
- eOpenOptionReadWrite | eOpenOptionCanCreate |
- eOpenOptionTruncate)
-  .Cases("a+", "ab+", "a+b",
- eOpenOptionReadWrite | eOpenOptionAppend |
- eOpenOptionCanCreate)
-  .Default(eOpenOptionInvalid);
+  OpenOptions opts = llvm::StringSwitch(mode)
+ .Cases("r", "rb", eOpenOptionReadOnly)
+ .Cases("w", "wb", eOpenOptionWriteOnly)
+ .Cases("a", "ab",
+eOpenOptionWriteOnly | eOpenOptionAppend |
+eOpenOptionCanCreate)
+ .Cases("r+", "rb+", "r+b", eOpenOptionReadWrite)
+ .Cases("w+", "wb+", "w+b",
+eOpenOptionReadWrite | eOpenOptionCanCreate |
+eOpenOptionTruncate)
+ .Cases("a+", "ab+", "a+b",
+eOpenOptionReadWrite | eOpenOptionAppend |
+eOpenOptionCanCreate)
+ .Default(eOpenOptionInvalid);
   if (opts != eOpenOptionInvalid)
 return opts;
   return llvm::createStringError(
@@ -219,7 +218,8 @@
 size_t File::PrintfVarArg(const char *format, va_list args) {
   llvm::SmallString<0> s;
   if (VASprintf(s, format, args)) {
-size_t written = s.size();;
+size_t written = s.size();
+;
 Write(s.data(), written);
 return written;
   }
@@ -247,15 +247,20 @@
   return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
 }
 
+bool NativeFile::IsValid() const {
+  return DescriptorIsValid() || StreamIsValid();
+}
+
 Expected NativeFile::GetOptions() const { return m_options; }
 
 int NativeFile::GetDescriptor() const {
-  if (DescriptorIsValid())
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
 return m_descriptor;
+  }
 
   // Don't open the file descriptor if we don't need to, just get it from the
   // stream if we have one.
-  if (StreamIsValid()) {
+  if (ValueGuard stream_guard = StreamIsValid()) {
 #if defined(_WIN32)
 return _fileno(m_stream);
 #else
@@ -272,8 +277,9 @@
 }
 
 FILE *NativeFile::GetStream() {
-  if (!StreamIsValid()) {
-if (DescriptorIsValid()) {
+  ValueGuard stream_guard = StreamIsValid();
+  if (!stream_guard) {
+if (ValueGuard descriptor_guard = DescriptorIsValid()) {
   auto mode = GetStreamOpenModeFromOptions(m_options);
   if (!mode)
 llvm::consumeError(mode.takeError());
@@ -307,30 +313,39 @@
 
 Status NativeFile::Close() {
   Status error;
-  if (StreamIsValid()) {
-if (m_own_stream) {
-  if (::fclose(m_stream) == EOF)
-error.SetErrorToErrno();
-} else {
-  File::OpenOptions rw =
-  m_options & (File::eOpenOptionReadOnly | File::eOpenOptionWriteOnly |
-   File::eOpenOptionReadWrite);
 
-  if (rw == eOpenOptionWriteOnly || rw == eOpenOptionReadWrite) {
-if (::fflush(m_stream) == EOF)
+  {
+ValueGuard stream_guard = StreamIsValid();
+if (stream_guard) {
+  if (m_own_stream) {
+if (::fclose(m_stream) == EOF)
   error.SetErrorToErrno();
+  } else {
+File::OpenOptions rw = m_options & (File::eOpenOptionReadOnly |
+File::eOpenOptionWriteOnly |
+File::eOpenOptionReadWrite);
+
+if (rw == eOpenOptionWriteOnly || rw == eOpenOptionReadWrite) {
+  if (::fflush(m_stream) == EOF)
+error.SetErrorToErrno();
+}
   }
 }
+m_stream = kInvalidStream;
+m_own_stream = false;
   }
-  if (DescriptorIsValid() && m_own_descriptor) {
-if (::close(m_descriptor) != 0)
-  error.SetErrorToErrno();
+
+  {
+ValueGuard descriptor_guard = DescriptorIsValid();
+if (descriptor_guard && m_own_descriptor) {
+  if (::close(m_descriptor) != 0)
+error.SetErrorToErrno();
+}
+m_descriptor = kInvalidDescriptor;
+m_own_descriptor = false;
   }
-  m_descriptor = kInvalidDescriptor;
-  m_

[Lldb-commits] [lldb] caa5167 - Revert "[lldb] Fix data race in ConnectionFileDescriptor"

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-08-07T19:11:11-07:00
New Revision: caa5167769b5d8a0165e6c1cb7c919e864346db2

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

LOG: Revert "[lldb] Fix data race in ConnectionFileDescriptor"

This reverts commit 0bdbe7bd7f1589817495a60cc8422df49575b17b because it
broke the bots.

Added: 


Modified: 
lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h 
b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
index 8c8424ed48154b..35773d5907e913 100644
--- a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@ class ConnectionFileDescriptor : public Connection {
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  mutable std::recursive_mutex m_mutex;
+  std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.

diff  --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp 
b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 9bb0268b2a704c..6a367a3307e543 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,7 +118,6 @@ void ConnectionFileDescriptor::CloseCommandPipe() {
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
-  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 



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

[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in ConnectionFileDescriptor

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0bdbe7bd7f15: [lldb] Fix data race in 
ConnectionFileDescriptor (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157347

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


Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 
Index: lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
===
--- lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.


Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 
Index: lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
===
--- lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 0bdbe7b - [lldb] Fix data race in ConnectionFileDescriptor

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-08-07T18:37:52-07:00
New Revision: 0bdbe7bd7f1589817495a60cc8422df49575b17b

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

LOG: [lldb] Fix data race in ConnectionFileDescriptor

TSan reports the following data race:

  Write of size 4 at 0x000109e0b160 by thread T2 (mutexes: write M0, write M1):
#0 NativeFile::Close() File.cpp:329
#1 ConnectionFileDescriptor::Disconnect(lldb_private::Status*) 
ConnectionFileDescriptorPosix.cpp:232
#2 Communication::Disconnect(lldb_private::Status*) Communication.cpp:61
#3 process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164
#4 Process::SetExitStatus(int, char const*) Process.cpp:1097
#5 process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(...) 
ProcessGDBRemote.cpp:3387

  Previous read of size 4 at 0x000109e0b160 by main thread (mutexes: write M2):
#0 NativeFile::IsValid() const File.h:393
#1 ConnectionFileDescriptor::IsConnected() const 
ConnectionFileDescriptorPosix.cpp:121
#2 Communication::IsConnected() const Communication.cpp:79
#3 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) 
GDBRemoteCommunication.cpp:256
#4 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...l) 
GDBRemoteCommunication.cpp:244
#5 
process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef,
 StringExtractorGDBRemote&) GDBRemoteClientBase.cpp:246

The problem is that in WaitForPacketNoLock's run loop, it checks that
the connection is still connected. This races with the
ConnectionFileDescriptor disconnecting. Most (but not all) access to the
IOObject in ConnectionFileDescriptorPosix is already gated by the mutex.
This patch just protects IsConnected in the same way.

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

Added: 


Modified: 
lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h 
b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
index 35773d5907e913..8c8424ed48154b 100644
--- a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@ class ConnectionFileDescriptor : public Connection {
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.

diff  --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp 
b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 6a367a3307e543..9bb0268b2a704c 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@ void ConnectionFileDescriptor::CloseCommandPipe() {
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 



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

[Lldb-commits] [PATCH] D157160: Surface error messages from the DebugSymbols DBGShellCommands external agent; add logging for LC_NOTEs in Mach-O corefiles

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 548015.
jasonmolenda added a comment.

Update patch to send these error messages to the ErrorStream not OutputStream.  
Also change LocateSymbolFileMacOSX.cpp so the full command that was used to 
find the binary is included in the error message, to make it clearer exactly 
where the error message originated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157160

Files:
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -330,7 +330,8 @@
 
 static bool GetModuleSpecInfoFromUUIDDictionary(CFDictionaryRef uuid_dict,
 ModuleSpec &module_spec,
-Status &error) {
+Status &error,
+const std::string &command) {
   Log *log = GetLog(LLDBLog::Host);
   bool success = false;
   if (uuid_dict != NULL && CFGetTypeID(uuid_dict) == CFDictionaryGetTypeID()) {
@@ -342,7 +343,10 @@
CFSTR("DBGError"));
 if (cf_str && CFGetTypeID(cf_str) == CFStringGetTypeID()) {
   if (CFCString::FileSystemRepresentation(cf_str, str)) {
-error.SetErrorString(str);
+std::string errorstr = command;
+errorstr += ":\n";
+errorstr += str;
+error.SetErrorString(errorstr);
   }
 }
 
@@ -652,7 +656,8 @@
 CFCString uuid_cfstr(uuid_str.c_str());
 CFDictionaryRef uuid_dict =
 (CFDictionaryRef)CFDictionaryGetValue(plist.get(), uuid_cfstr.get());
-return GetModuleSpecInfoFromUUIDDictionary(uuid_dict, module_spec, error);
+return GetModuleSpecInfoFromUUIDDictionary(uuid_dict, module_spec, error,
+   command.GetData());
   }
 
   if (const CFIndex num_values = ::CFDictionaryGetCount(plist.get())) {
@@ -661,13 +666,14 @@
 ::CFDictionaryGetKeysAndValues(plist.get(), NULL,
(const void **)&values[0]);
 if (num_values == 1) {
-  return GetModuleSpecInfoFromUUIDDictionary(values[0], module_spec, error);
+  return GetModuleSpecInfoFromUUIDDictionary(values[0], module_spec, error,
+ command.GetData());
 }
 
 for (CFIndex i = 0; i < num_values; ++i) {
   ModuleSpec curr_module_spec;
   if (GetModuleSpecInfoFromUUIDDictionary(values[i], curr_module_spec,
-  error)) {
+  error, command.GetData())) {
 if (module_spec.GetArchitecture().IsCompatibleMatch(
 curr_module_spec.GetArchitecture())) {
   module_spec = curr_module_spec;
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -5414,6 +5414,8 @@
 
 std::string ObjectFileMachO::GetIdentifierString() {
   std::string result;
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5449,6 +5451,8 @@
 result = buf;
 if (buf)
   free(buf);
+LLDB_LOGF(log, "LC_NOTE 'kern ver str' found with text '%s'",
+  result.c_str());
 return result;
   }
 }
@@ -5472,6 +5476,7 @@
   buf) == ident_command.cmdsize) {
   buf[ident_command.cmdsize - 1] = '\0';
   result = buf;
+  LLDB_LOGF(log, "LC_IDENT found with text '%s'", result.c_str());
 }
 if (buf)
   free(buf);
@@ -5484,6 +5489,7 @@
 
 addr_t ObjectFileMachO::GetAddressMask() {
   addr_t mask = 0;
+  Log *log(GetLog(LLDBLog::Process));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5511,6 +5517,10 @@
   if (num_addr_bits != 0) {
 mask = ~((1ULL << num_addr_bits) - 1);
   }
+  LLDB_LOGF(log,
+"LC_NOTE 'addrable bits' found, value %d bits, "
+"mask 0x%" PRIx64,
+num_addr_bits, mask);
   break;
 }
  

[Lldb-commits] [PATCH] D157160: Surface error messages from the DebugSymbols DBGShellCommands external agent; add logging for LC_NOTEs in Mach-O corefiles

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/source/Core/DynamicLoader.cpp:241-243
+Stream &s = target.GetDebugger().GetOutputStream();
+s.Printf("Tried DBGShellCommands cmd, got error: %s\n",
+ error.AsCString());

JDevlieghere wrote:
> Shouldn't this be the error stream?
Ah good suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157160

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

[Lldb-commits] [PATCH] D157160: Surface error messages from the DebugSymbols DBGShellCommands external agent; add logging for LC_NOTEs in Mach-O corefiles

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 548009.
jasonmolenda added a comment.

fix typeo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157160

Files:
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -5414,6 +5414,8 @@
 
 std::string ObjectFileMachO::GetIdentifierString() {
   std::string result;
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5449,6 +5451,8 @@
 result = buf;
 if (buf)
   free(buf);
+LLDB_LOGF(log, "LC_NOTE 'kern ver str' found with text '%s'",
+  result.c_str());
 return result;
   }
 }
@@ -5472,6 +5476,7 @@
   buf) == ident_command.cmdsize) {
   buf[ident_command.cmdsize - 1] = '\0';
   result = buf;
+  LLDB_LOGF(log, "LC_IDENT found with text '%s'", result.c_str());
 }
 if (buf)
   free(buf);
@@ -5484,6 +5489,7 @@
 
 addr_t ObjectFileMachO::GetAddressMask() {
   addr_t mask = 0;
+  Log *log(GetLog(LLDBLog::Process));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5511,6 +5517,10 @@
   if (num_addr_bits != 0) {
 mask = ~((1ULL << num_addr_bits) - 1);
   }
+  LLDB_LOGF(log,
+"LC_NOTE 'addrable bits' found, value %d bits, "
+"mask 0x%" PRIx64,
+num_addr_bits, mask);
   break;
 }
   }
@@ -5526,6 +5536,8 @@
 bool &value_is_offset,
 UUID &uuid,
 ObjectFile::BinaryType &type) {
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   value = LLDB_INVALID_ADDRESS;
   value_is_offset = false;
   uuid.Clear();
@@ -5605,20 +5617,31 @@
   uuid = UUID(raw_uuid, sizeof(uuid_t));
   // convert the "main bin spec" type into our
   // ObjectFile::BinaryType enum
+  const char *typestr = "unrecognized type";
   switch (binspec_type) {
   case 0:
 type = eBinaryTypeUnknown;
+typestr = "uknown";
 break;
   case 1:
 type = eBinaryTypeKernel;
+typestr = "xnu kernel";
 break;
   case 2:
 type = eBinaryTypeUser;
+typestr = "userland dyld";
 break;
   case 3:
 type = eBinaryTypeStandalone;
+typestr = "standalone";
 break;
   }
+  LLDB_LOGF(log,
+"LC_NOTE 'main bin spec' found, version %d type %d "
+"(%s), value 0x%" PRIx64 " value-is-slide==%s uuid %s",
+version, type, typestr, value,
+value_is_offset ? "true" : "false",
+uuid.GetAsString().c_str());
   if (!m_data.GetU32(&offset, &log2_pagesize, 1))
 return false;
   if (version > 1 && !m_data.GetU32(&offset, &platform, 1))
@@ -6811,6 +6834,8 @@
 ObjectFileMachO::MachOCorefileAllImageInfos
 ObjectFileMachO::GetCorefileAllImageInfos() {
   MachOCorefileAllImageInfos image_infos;
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
 
   // Look for an "all image infos" LC_NOTE.
   lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
@@ -6840,6 +6865,9 @@
 //  offset += 4; // uint32_t entries_size;
 //  offset += 4; // uint32_t unused;
 
+LLDB_LOGF(log,
+  "LC_NOTE 'all image infos' found version %d with %d images",
+  version, imgcount);
 offset = entries_fileoff;
 for (uint32_t i = 0; i < imgcount; i++) {
   // Read the struct image_entry.
@@ -6871,6 +6899,12 @@
 vmaddr};
 image_entry.segment_load_addresses.push_back(new_seg);
   }
+  LLDB_LOGF(
+  log, "  image entry: %s %s 0x%" PRIx64 " %s",
+  image_entry.filename.c_str(),
+

[Lldb-commits] [PATCH] D157160: Surface error messages from the DebugSymbols DBGShellCommands external agent; add logging for LC_NOTEs in Mach-O corefiles

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Core/DynamicLoader.cpp:241-243
+Stream &s = target.GetDebugger().GetOutputStream();
+s.Printf("Tried DBGShellCommands cmd, got error: %s\n",
+ error.AsCString());

Shouldn't this be the error stream?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157160

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

[Lldb-commits] [PATCH] D157160: Surface error messages from the DebugSymbols DBGShellCommands external agent; add logging for LC_NOTEs in Mach-O corefiles

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 548001.
jasonmolenda added a comment.

Change calls to Log::Printf to use LLDB_LOGF().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157160

Files:
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -5414,6 +5414,8 @@
 
 std::string ObjectFileMachO::GetIdentifierString() {
   std::string result;
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5449,6 +5451,8 @@
 result = buf;
 if (buf)
   free(buf);
+LLDB_LOGF("LC_NOTE 'kern ver str' found with text '%s'",
+  result.c_str());
 return result;
   }
 }
@@ -5472,6 +5476,7 @@
   buf) == ident_command.cmdsize) {
   buf[ident_command.cmdsize - 1] = '\0';
   result = buf;
+  LLDB_LOGF("LC_IDENT found with text '%s'", result.c_str());
 }
 if (buf)
   free(buf);
@@ -5484,6 +5489,7 @@
 
 addr_t ObjectFileMachO::GetAddressMask() {
   addr_t mask = 0;
+  Log *log(GetLog(LLDBLog::Process));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
@@ -5511,6 +5517,9 @@
   if (num_addr_bits != 0) {
 mask = ~((1ULL << num_addr_bits) - 1);
   }
+  LLDB_LOGF("LC_NOTE 'addrable bits' found, value %d bits, "
+"mask 0x%" PRIx64,
+num_addr_bits, mask);
   break;
 }
   }
@@ -5526,6 +5535,8 @@
 bool &value_is_offset,
 UUID &uuid,
 ObjectFile::BinaryType &type) {
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
   value = LLDB_INVALID_ADDRESS;
   value_is_offset = false;
   uuid.Clear();
@@ -5605,20 +5616,30 @@
   uuid = UUID(raw_uuid, sizeof(uuid_t));
   // convert the "main bin spec" type into our
   // ObjectFile::BinaryType enum
+  const char *typestr = "unrecognized type";
   switch (binspec_type) {
   case 0:
 type = eBinaryTypeUnknown;
+typestr = "uknown";
 break;
   case 1:
 type = eBinaryTypeKernel;
+typestr = "xnu kernel";
 break;
   case 2:
 type = eBinaryTypeUser;
+typestr = "userland dyld";
 break;
   case 3:
 type = eBinaryTypeStandalone;
+typestr = "standalone";
 break;
   }
+  LLDB_LOGF("LC_NOTE 'main bin spec' found, version %d type %d "
+"(%s), value 0x%" PRIx64 " value-is-slide==%s uuid %s",
+version, type, typestr, value,
+value_is_offset ? "true" : "false",
+uuid.GetAsString().c_str());
   if (!m_data.GetU32(&offset, &log2_pagesize, 1))
 return false;
   if (version > 1 && !m_data.GetU32(&offset, &platform, 1))
@@ -6811,6 +6832,8 @@
 ObjectFileMachO::MachOCorefileAllImageInfos
 ObjectFileMachO::GetCorefileAllImageInfos() {
   MachOCorefileAllImageInfos image_infos;
+  Log *log(
+  GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));
 
   // Look for an "all image infos" LC_NOTE.
   lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
@@ -6840,6 +6863,8 @@
 //  offset += 4; // uint32_t entries_size;
 //  offset += 4; // uint32_t unused;
 
+LLDB_LOGF("LC_NOTE 'all image infos' found version %d with %d images",
+  version, imgcount);
 offset = entries_fileoff;
 for (uint32_t i = 0; i < imgcount; i++) {
   // Read the struct image_entry.
@@ -6871,6 +6896,12 @@
 vmaddr};
 image_entry.segment_load_addresses.push_back(new_seg);
   }
+  LLDB_LOGF(
+  "  image entry: %s %s 0x%" PRIx64 " %s",
+  image_entry.filename.c_str(),
+  image_entry.uuid.GetAsString().c_str(), image_en

[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in ConnectionFileDescriptor

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

Makes sense to me.


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

https://reviews.llvm.org/D157347

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

[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in ConnectionFileDescriptor

[Augusto Noronha via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D157347

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

[Lldb-commits] [lldb] 300c5aa - [lldb] Remove unused Socket::PreDisconnect (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-08-07T15:50:06-07:00
New Revision: 300c5aaa0a2ae3b5bb7d14d4f11d93e4ea2666c1

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

LOG: [lldb] Remove unused Socket::PreDisconnect (NFC)

Added: 


Modified: 
lldb/include/lldb/Host/Socket.h
lldb/source/Host/common/Socket.cpp
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 01f790ee11fb8c..4dad88e78faa11 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -93,7 +93,6 @@ class Socket : public IOObject {
   Status Read(void *buf, size_t &num_bytes) override;
   Status Write(const void *buf, size_t &num_bytes) override;
 
-  virtual Status PreDisconnect();
   Status Close() override;
 
   bool IsValid() const override { return m_socket != kInvalidSocketValue; }

diff  --git a/lldb/source/Host/common/Socket.cpp 
b/lldb/source/Host/common/Socket.cpp
index 8ccb27fdd7f452..bd0c127a08956e 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -269,11 +269,6 @@ Status Socket::Write(const void *buf, size_t &num_bytes) {
   return error;
 }
 
-Status Socket::PreDisconnect() {
-  Status error;
-  return error;
-}
-
 Status Socket::Close() {
   Status error;
   if (!IsValid() || !m_should_close_fd)

diff  --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp 
b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 825da09d760224..6a367a3307e543 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -202,9 +202,6 @@ ConnectionStatus 
ConnectionFileDescriptor::Disconnect(Status *error_ptr) {
 return eConnectionStatusSuccess;
   }
 
-  if (m_io_sp->GetFdType() == IOObject::eFDTypeSocket)
-static_cast(*m_io_sp).PreDisconnect();
-
   // Try to get the ConnectionFileDescriptor's mutex.  If we fail, that is
   // quite likely because somebody is doing a blocking read on our file
   // descriptor.  If that's the case, then send the "q" char to the command



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

[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in ConnectionFileDescriptor

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, augusto2112.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
JDevlieghere requested review of this revision.

TSan reports the following data race:

  Write of size 4 at 0x000109e0b160 by thread T2 (mutexes: write M0, write M1):
#0 lldb_private::NativeFile::Close() File.cpp:329 
(liblldb.18.0.0git.dylib:arm64+0x58dac0)
#1 
lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) 
ConnectionFileDescriptorPosix.cpp:232 (liblldb.18.0.0git.dylib:arm64+0x5bad6c)
#2 lldb_private::Communication::Disconnect(lldb_private::Status*) 
Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x451f04)
#3 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() 
ProcessGDBRemote.cpp:1164 (liblldb.18.0.0git.dylib:arm64+0xadfd80)
#4 lldb_private::Process::SetExitStatus(int, char const*) Process.cpp:1097 
(liblldb.18.0.0git.dylib:arm64+0x6ed338)
#5 
lldb_private::process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(std::__1::weak_ptr,
 unsigned long long, int, int) ProcessGDBRemote.cpp:3387 
(liblldb.18.0.0git.dylib:arm64+0xae9464)
  
  Previous read of size 4 at 0x000109e0b160 by main thread (mutexes: write M2):
#0 lldb_private::NativeFile::IsValid() const File.h:393 
(liblldb.18.0.0git.dylib:arm64+0x590830)
#1 lldb_private::ConnectionFileDescriptor::IsConnected() const 
ConnectionFileDescriptorPosix.cpp:121 (liblldb.18.0.0git.dylib:arm64+0x5b8ab4)
#2 lldb_private::Communication::IsConnected() const Communication.cpp:79 
(liblldb.18.0.0git.dylib:arm64+0x451fcc)
#3 
lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&,
 lldb_private::Timeout>, bool) 
GDBRemoteCommunication.cpp:256 (liblldb.18.0.0git.dylib:arm64+0xaac060)
#4 
lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&,
 lldb_private::Timeout>, bool) 
GDBRemoteCommunication.cpp:244 (liblldb.18.0.0git.dylib:arm64+0xaabe78)
#5 
lldb_private::process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef,
 StringExtractorGDBRemote&) GDBRemoteClientBase.cpp:246 
(liblldb.18.0.0git.dylib:arm64+0xaaa184)

The problem is that in `WaitForPacketNoLock`'s run loop, it checks that the 
connection is still connected. This races with the `ConnectionFileDescriptor` 
disconnecting. Most (but not all) access to the `IOObject` in 
`ConnectionFileDescriptorPosix` is already gated by the mutex. This patch just 
protects `IsConnected` in the same way.


https://reviews.llvm.org/D157347

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


Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 
Index: lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
===
--- lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.


Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }
 
Index: lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
===
--- lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic m_shutting_down; // This marks that we are shutting down so
  // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-comm

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG57cbd26a68ab: Flag for LoadBinaryWithUUIDAndAddress, to 
create memory image or not (authored by jasonmolenda).

Changed prior to commit:
  https://reviews.llvm.org/D157167?vs=547866&id=547978#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp

Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
@@ -16,7 +16,7 @@
 #include 
 
 // Given a list of binaries, and optional slides to be applied,
-// create a corefile whose memory is those binaries laid at at
+// create a corefile whose memory is those binaries laid down at
 // their slid addresses.
 //
 // Add a 'main bin spec' LC_NOTE for the first binary, and
Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
@@ -1,4 +1,4 @@
-"""Test corefiles with "main bin spec"/"load binary" with only addrs work."""
+"""Test corefiles with "main bin spec"/"load binary" with only vmaddrs works."""
 
 
 import os
@@ -35,41 +35,28 @@
 self.libtwo_exe,
 self.libtwo_slide,
 )
+if self.TraceOn():
+print("Creating corefile with command %s")
 call(cmd, shell=True)
 
 def load_corefile_and_test(self):
 target = self.dbg.CreateTarget("")
 err = lldb.SBError()
 if self.TraceOn():
-self.runCmd("script print('loading corefile %s')" % self.corefile)
+print("loading corefile %s" % self.corefile)
 process = target.LoadCore(self.corefile)
 self.assertEqual(process.IsValid(), True)
 if self.TraceOn():
-self.runCmd("script print('image list after loading corefile:')")
+print("image list after loading corefile:")
 self.runCmd("image list")
 
-self.assertEqual(target.GetNumModules(), 3)
+## We don't have libone.dylib in the global module cache or from
+## dsymForUUID, and lldb will not read the binary out of memory.
+self.assertEqual(target.GetNumModules(), 2)
 fspec = target.GetModuleAtIndex(0).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.aout_exe_basename)
 
-# libone.dylib was never loaded into lldb, see that we added a memory module.
 fspec = target.GetModuleAtIndex(1).GetFileSpec()
-self.assertIn("memory-image", fspec.GetFilename())
-
-dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
-dwarfdump_cmd_output = subprocess.check_output(
-('/usr/bin/dwarfdump --uuid "%s"' % self.libone_exe), shell=True
-).decode("utf-8")
-libone_uuid = None
-for line in dwarfdump_cmd_output.splitlines():
-match = dwarfdump_uuid_regex.search(line)
-if match:
-libone_uuid = match.group(1)
-
-memory_image_uuid = target.GetModuleAtIndex(1).GetUUIDString()
-self.assertEqual(libone_uuid, memory_image_uuid)
-
-fspec = target.GetModuleAtIndex(2).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.libtwo_exe_basename)
 
 # Executables "always" have this base address
@@ -80,17 +67,9 @@
 )
 self.assertEqual(aout_load, 0x1 + self.aout_slide)
 
-# Value from Makefile
-libone_load = (
-target.GetModuleAtIndex(1)
-.GetObjectFileHeaderAddress()
-.GetLoadAddress(target)
-)
-self.assertEqual(libone_load, self.libone_slide)
-
 # Value from Makefile
 libtwo_load = (
-target.GetModuleAtIndex(2)
+target.GetModuleAtIndex(1)
 .GetObjectFileHeaderAddress()
 .GetLoadAddress(target)
 )
@@ -140,6 +119,15 @@
 self.assertNotEqual(
 libtwo_uuid, None, "Could not get uuid of built libtwo.dylib"
 )
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr

[Lldb-commits] [lldb] 57cbd26 - Flag for LoadBinaryWithUUIDAndAddress, to create memory image or not

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-08-07T15:19:45-07:00
New Revision: 57cbd26a68ab61631f5f4272d3c90df2eb0ce4f6

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

LOG: Flag for LoadBinaryWithUUIDAndAddress, to create memory image or not

DynamicLoader::LoadBinaryWithUUIDAndAddress can create a Module based
on the binary image in memory, which in some cases contains symbol
names and can be genuinely useful.  If we don't have a filename, it
creates a name in the form `memory-image-0x...` with the header address.

In practice, this is most useful with Darwin userland corefiles
where the binary was stored in the corefile in whole, and we can't
find a binary with the matching UUID.  Using the binary out of
the corefile memory in this case works well.

But in other cases, akin to firmware debugging, we merely end up
with an oddly named binary image and no symbols.

Add a flag to control whether we will create these memory images
and add them to the Target or not; only set it to true when working
with a userland Mach-O image with the "all image infos" LC_NOTE for
a userland corefile.

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

Added: 


Modified: 
lldb/include/lldb/Target/DynamicLoader.h
lldb/source/Core/DynamicLoader.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/DynamicLoader.h 
b/lldb/include/lldb/Target/DynamicLoader.h
index 3aa92398d0130d..e508d192d27598 100644
--- a/lldb/include/lldb/Target/DynamicLoader.h
+++ b/lldb/include/lldb/Target/DynamicLoader.h
@@ -264,13 +264,18 @@ class DynamicLoader : public PluginInterface {
   /// load address for the binary or its segments in the Target if it 
passes
   /// true.
   ///
+  /// \param[in] allow_memory_image_last_resort
+  /// If no better binary image can be found, allow reading the binary
+  /// out of memory, if possible, and create the Module based on that.
+  /// May be slow to read a binary out of memory, and for unusual
+  /// environments, may be no symbols mapped in memory at all.
+  ///
   /// \return
   /// Returns a shared pointer for the Module that has been added.
-  static lldb::ModuleSP
-  LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
-   UUID uuid, lldb::addr_t value,
-   bool value_is_offset, bool force_symbol_search,
-   bool notify, bool set_address_in_target);
+  static lldb::ModuleSP LoadBinaryWithUUIDAndAddress(
+  Process *process, llvm::StringRef name, UUID uuid, lldb::addr_t value,
+  bool value_is_offset, bool force_symbol_search, bool notify,
+  bool set_address_in_target, bool allow_memory_image_last_resort);
 
   /// Get information about the shared cache for a process, if possible.
   ///

diff  --git a/lldb/source/Core/DynamicLoader.cpp 
b/lldb/source/Core/DynamicLoader.cpp
index 2e5378f654a51a..0d138d7bb34584 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -188,7 +188,7 @@ static ModuleSP ReadUnnamedMemoryModule(Process *process, 
addr_t addr,
 ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
 Process *process, llvm::StringRef name, UUID uuid, addr_t value,
 bool value_is_offset, bool force_symbol_search, bool notify,
-bool set_address_in_target) {
+bool set_address_in_target, bool allow_memory_image_last_resort) {
   ModuleSP memory_module_sp;
   ModuleSP module_sp;
   PlatformSP platform_sp = process->GetTarget().GetPlatform();
@@ -245,7 +245,8 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
 
   // If we couldn't find the binary anywhere else, as a last resort,
   // read it out of memory.
-  if (!module_sp.get() && value != LLDB_INVALID_ADDRESS && !value_is_offset) {
+  if (allow_memory_image_last_resort && !module_sp.get() &&
+  value != LLDB_INVALID_ADDRESS && !value_is_offset) {
 if (!memory_module_sp)
   memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
 if (memory_module_sp)

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 56e1726aa5710c..1f200923f37d5f 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6938,9 +6938,15 @@ bool 
ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &

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

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM




Comment at: lldb/source/Core/Communication.cpp:115-118
+total_written +=
+WriteUnlocked(static_cast(src) + total_written,
+  src_len - total_written, status, error_ptr);
   while (status == eConnectionStatusSuccess && total_written < src_len);

Unrelated to your patch but something about this is triggering my spider sense. 
Is it possible that `status == eConnectionStatusSuccess` while `WriteUnlocked` 
returns 0? This might end up in an infinite loop then, no?

Obviously you don't need to address this since it's unrelated, but something to 
think about for resiliency.


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

https://reviews.llvm.org/D157159

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

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[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/D157167/new/

https://reviews.llvm.org/D157167

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

[Lldb-commits] [PATCH] D157168: [lldb] [mach-o corefiles] If we have LC_NOTE metadata and can't find a binary, don't fall back to an exhaustive scan

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.h:89
   void CreateMemoryRegions();
-  void LoadBinariesViaMetadata();
+  bool LoadBinariesViaMetadata();
   void LoadBinariesViaExhaustiveSearch();

bulbazord wrote:
> Since `LoadBinariesViaMetadata` can fail and we now care about the return 
> value, what do you think about the name `TryLoadBinariesViaMetadata`? I think 
> a name that indicates that it may fail to actually load would make it easier 
> to read at a glance.
The same is true for the other methods though, so I don't know if that is 
really all that meaningful. IIUC, before this patch, all these methods conveyed 
whether they were successful by setting `m_dyld_plugin_name`. Now, 
`LoadBinariesViaMetadata` does something extra, which is conveyed by the 
boolean. I dislike the inconsistency, but I can't really think of a better way 
of doing this, I considered suggesting an out-parameter but that doesn't really 
improve anything. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157168

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

[Lldb-commits] [PATCH] D157152: [lldb] Make TSan errors fatal when running the test suite

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG17226c976e04: [lldb] Make TSan errors fatal when running the 
test suite (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157152

Files:
  lldb/test/API/lit.cfg.py
  lldb/test/Shell/lit.cfg.py
  lldb/test/Unit/lit.cfg.py


Index: lldb/test/Unit/lit.cfg.py
===
--- lldb/test/Unit/lit.cfg.py
+++ lldb/test/Unit/lit.cfg.py
@@ -34,5 +34,9 @@
 )
 llvm_config.with_environment("PATH", os.path.dirname(sys.executable), 
append_path=True)
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # testFormat: The test format to use to interpret tests.
 config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Tests")
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -48,6 +48,10 @@
 ]
 )
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # Support running the test suite under the lldb-repro wrapper. This makes it
 # possible to capture a test suite run and then rerun all the test from the
 # just captured reproducer.
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -123,6 +123,7 @@
 )
 
 if "Thread" in config.llvm_use_sanitizer:
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
 if "Darwin" in config.host_os:
 config.environment["DYLD_INSERT_LIBRARIES"] = 
find_sanitizer_runtime(
 "libclang_rt.tsan_osx_dynamic.dylib"


Index: lldb/test/Unit/lit.cfg.py
===
--- lldb/test/Unit/lit.cfg.py
+++ lldb/test/Unit/lit.cfg.py
@@ -34,5 +34,9 @@
 )
 llvm_config.with_environment("PATH", os.path.dirname(sys.executable), append_path=True)
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # testFormat: The test format to use to interpret tests.
 config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Tests")
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -48,6 +48,10 @@
 ]
 )
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # Support running the test suite under the lldb-repro wrapper. This makes it
 # possible to capture a test suite run and then rerun all the test from the
 # just captured reproducer.
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -123,6 +123,7 @@
 )
 
 if "Thread" in config.llvm_use_sanitizer:
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
 if "Darwin" in config.host_os:
 config.environment["DYLD_INSERT_LIBRARIES"] = find_sanitizer_runtime(
 "libclang_rt.tsan_osx_dynamic.dylib"
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 17226c9 - [lldb] Make TSan errors fatal when running the test suite

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-08-07T13:20:38-07:00
New Revision: 17226c976e04856b10e2da34e93c89596660f882

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

LOG: [lldb] Make TSan errors fatal when running the test suite

Set the halt_on_error runtime flag to make TSan errors fatal when
running the test suite. For the API tests the environment variables are
set conditionally on whether the TSan is enabled. The Shell and Unit
tests don't have that logic but setting the environment variable is
harmless. For consistency, I've also mirrored the ASAN option
(detect_stack_use_after_return=1) for the Shell tests.

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

Added: 


Modified: 
lldb/test/API/lit.cfg.py
lldb/test/Shell/lit.cfg.py
lldb/test/Unit/lit.cfg.py

Removed: 




diff  --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index 167ab3004c4066..c306c5b3c0ec0c 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -123,6 +123,7 @@ def delete_module_cache(path):
 )
 
 if "Thread" in config.llvm_use_sanitizer:
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
 if "Darwin" in config.host_os:
 config.environment["DYLD_INSERT_LIBRARIES"] = 
find_sanitizer_runtime(
 "libclang_rt.tsan_osx_dynamic.dylib"

diff  --git a/lldb/test/Shell/lit.cfg.py b/lldb/test/Shell/lit.cfg.py
index fdf5cf9bfd69a7..d75c1f532e147f 100644
--- a/lldb/test/Shell/lit.cfg.py
+++ b/lldb/test/Shell/lit.cfg.py
@@ -48,6 +48,10 @@
 ]
 )
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # Support running the test suite under the lldb-repro wrapper. This makes it
 # possible to capture a test suite run and then rerun all the test from the
 # just captured reproducer.

diff  --git a/lldb/test/Unit/lit.cfg.py b/lldb/test/Unit/lit.cfg.py
index 4d38b0088bc3b1..1dda6569a7b3cc 100644
--- a/lldb/test/Unit/lit.cfg.py
+++ b/lldb/test/Unit/lit.cfg.py
@@ -34,5 +34,9 @@
 )
 llvm_config.with_environment("PATH", os.path.dirname(sys.executable), 
append_path=True)
 
+# Enable sanitizer runtime flags.
+config.environment["ASAN_OPTIONS"] = "detect_stack_use_after_return=1"
+config.environment["TSAN_OPTIONS"] = "halt_on_error=1"
+
 # testFormat: The test format to use to interpret tests.
 config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Tests")



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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D151683#4566907 , @eaeltsin wrote:

> Hi,
>
> Is it a known issue, that clang doesn't compile `void foo(char *argv[] 
> [[maybe_unused]]) {}` ? The error is `error: 'maybe_unused' attribute cannot 
> be applied to types`.
>
> https://godbolt.org/z/r9E81cWxh - clang fails, but gcc doesn't.
>
> It looks like there is a lot of oss code of the form `void foo(char *argv[] 
> ATTRIBUTE_UNUSED)`, where `ATTRIBUTE_UNUSED` was configured to 
> `__attribute__((unused))` before this change (thus compiled ok) and to 
> `[[maybe_unused]]` after this change (thus started to break).

https://eel.is/c++draft/dcl.array#3.sentence-2 -- an attribute in that position 
appertains to the array type and `maybe_unused` cannot be applied to types in 
that way, so I believe Clang's behavior is correct. So I think the C++ 
attribute behavior is expected, but the change between `__attribute__` and 
`[[]]` may be catching folks off-guard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Evgeny Eltsin via Phabricator via lldb-commits]

eaeltsin added a comment.

Hi,

Is it a known issue, that clang doesn't compile `void foo(char *argv[] 
[[maybe_unused]]) {}` ?

https://godbolt.org/z/r9E81cWxh - clang fails, but gcc doesn't.

It looks like there is a lot of oss code of the form `void foo(char *argv[] 
ATTRIBUTE_UNUSED)`, where `ATTRIBUTE_UNUSED` was configured to 
`__attribute__((unused))` before this change (thus compiled ok) and to 
`[[maybe_unused]]` after this change (thus started to break).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 547866.
jasonmolenda added a comment.

Fix my `script print`'s to just `print`, I can't repo a buffering problem that 
I thought I had seen in the past.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp

Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
@@ -16,7 +16,7 @@
 #include 
 
 // Given a list of binaries, and optional slides to be applied,
-// create a corefile whose memory is those binaries laid at at
+// create a corefile whose memory is those binaries laid down at
 // their slid addresses.
 //
 // Add a 'main bin spec' LC_NOTE for the first binary, and
Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
@@ -1,4 +1,4 @@
-"""Test corefiles with "main bin spec"/"load binary" with only addrs work."""
+"""Test corefiles with "main bin spec"/"load binary" with only vmaddrs works."""
 
 
 import os
@@ -35,41 +35,28 @@
 self.libtwo_exe,
 self.libtwo_slide,
 )
+if self.TraceOn():
+print('Creating corefile with command %s')
 call(cmd, shell=True)
 
 def load_corefile_and_test(self):
 target = self.dbg.CreateTarget("")
 err = lldb.SBError()
 if self.TraceOn():
-self.runCmd("script print('loading corefile %s')" % self.corefile)
+print('loading corefile %s' % self.corefile)
 process = target.LoadCore(self.corefile)
 self.assertEqual(process.IsValid(), True)
 if self.TraceOn():
-self.runCmd("script print('image list after loading corefile:')")
+print('image list after loading corefile:')
 self.runCmd("image list")
 
-self.assertEqual(target.GetNumModules(), 3)
+## We don't have libone.dylib in the global module cache or from
+## dsymForUUID, and lldb will not read the binary out of memory.
+self.assertEqual(target.GetNumModules(), 2)
 fspec = target.GetModuleAtIndex(0).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.aout_exe_basename)
 
-# libone.dylib was never loaded into lldb, see that we added a memory module.
 fspec = target.GetModuleAtIndex(1).GetFileSpec()
-self.assertIn("memory-image", fspec.GetFilename())
-
-dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
-dwarfdump_cmd_output = subprocess.check_output(
-('/usr/bin/dwarfdump --uuid "%s"' % self.libone_exe), shell=True
-).decode("utf-8")
-libone_uuid = None
-for line in dwarfdump_cmd_output.splitlines():
-match = dwarfdump_uuid_regex.search(line)
-if match:
-libone_uuid = match.group(1)
-
-memory_image_uuid = target.GetModuleAtIndex(1).GetUUIDString()
-self.assertEqual(libone_uuid, memory_image_uuid)
-
-fspec = target.GetModuleAtIndex(2).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.libtwo_exe_basename)
 
 # Executables "always" have this base address
@@ -80,17 +67,9 @@
 )
 self.assertEqual(aout_load, 0x1 + self.aout_slide)
 
-# Value from Makefile
-libone_load = (
-target.GetModuleAtIndex(1)
-.GetObjectFileHeaderAddress()
-.GetLoadAddress(target)
-)
-self.assertEqual(libone_load, self.libone_slide)
-
 # Value from Makefile
 libtwo_load = (
-target.GetModuleAtIndex(2)
+target.GetModuleAtIndex(1)
 .GetObjectFileHeaderAddress()
 .GetLoadAddress(target)
 )
@@ -140,6 +119,15 @@
 self.assertNotEqual(
 libtwo_uuid, None, "Could not get uuid of built libtwo.dylib"
 )
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % self.aout_exe), shell=True
+).decode("utf-8")
+

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: 
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py:38-39
 )
+if self.TraceOn():
+self.runCmd("script print('Creating corefile with command %s')" % 
cmd)
 call(cmd, shell=True)

jasonmolenda wrote:
> bulbazord wrote:
> > JDevlieghere wrote:
> > > Out of curiosity what's the point of doing `self.runCmd("script print 
> > > ...` vs printing the same thing directly? Does this run in another 
> > > context or something? Do we save the command output somewhere? 
> > I'm also curious. Printing directly is probably less expensive than running 
> > a print command through the lldb python script interpreter.
> In the past, when I had an SB API test `print()` directly, the trace output 
> is buffered differently than STDOUT, so I've done this hack in a few places 
> to get the print to happen exactly when the test is executing.  Doing a `if 
> self.TraceOn(): self.runCmd("bt"); print("got here")` will not print the two 
> next to each other, iirc.  I can switch to a print and confirm my memory of 
> this.
I switched this test to just `print` directly and works fine with `lldb-dotest 
-t`, I must have misremembered.  I'll remove a few places where I do this in 
this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

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

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 547864.
jasonmolenda added a comment.

Incorporate Jonas' feedback about the naming of the new DynamicLoader method 
argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
  
lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp

Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp
@@ -16,7 +16,7 @@
 #include 
 
 // Given a list of binaries, and optional slides to be applied,
-// create a corefile whose memory is those binaries laid at at
+// create a corefile whose memory is those binaries laid down at
 // their slid addresses.
 //
 // Add a 'main bin spec' LC_NOTE for the first binary, and
Index: lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
===
--- lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
+++ lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py
@@ -1,4 +1,4 @@
-"""Test corefiles with "main bin spec"/"load binary" with only addrs work."""
+"""Test corefiles with "main bin spec"/"load binary" with only vmaddrs works."""
 
 
 import os
@@ -35,6 +35,8 @@
 self.libtwo_exe,
 self.libtwo_slide,
 )
+if self.TraceOn():
+self.runCmd("script print('Creating corefile with command %s')" % cmd)
 call(cmd, shell=True)
 
 def load_corefile_and_test(self):
@@ -48,28 +50,13 @@
 self.runCmd("script print('image list after loading corefile:')")
 self.runCmd("image list")
 
-self.assertEqual(target.GetNumModules(), 3)
+## We don't have libone.dylib in the global module cache or from
+## dsymForUUID, and lldb will not read the binary out of memory.
+self.assertEqual(target.GetNumModules(), 2)
 fspec = target.GetModuleAtIndex(0).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.aout_exe_basename)
 
-# libone.dylib was never loaded into lldb, see that we added a memory module.
 fspec = target.GetModuleAtIndex(1).GetFileSpec()
-self.assertIn("memory-image", fspec.GetFilename())
-
-dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
-dwarfdump_cmd_output = subprocess.check_output(
-('/usr/bin/dwarfdump --uuid "%s"' % self.libone_exe), shell=True
-).decode("utf-8")
-libone_uuid = None
-for line in dwarfdump_cmd_output.splitlines():
-match = dwarfdump_uuid_regex.search(line)
-if match:
-libone_uuid = match.group(1)
-
-memory_image_uuid = target.GetModuleAtIndex(1).GetUUIDString()
-self.assertEqual(libone_uuid, memory_image_uuid)
-
-fspec = target.GetModuleAtIndex(2).GetFileSpec()
 self.assertEqual(fspec.GetFilename(), self.libtwo_exe_basename)
 
 # Executables "always" have this base address
@@ -80,17 +67,9 @@
 )
 self.assertEqual(aout_load, 0x1 + self.aout_slide)
 
-# Value from Makefile
-libone_load = (
-target.GetModuleAtIndex(1)
-.GetObjectFileHeaderAddress()
-.GetLoadAddress(target)
-)
-self.assertEqual(libone_load, self.libone_slide)
-
 # Value from Makefile
 libtwo_load = (
-target.GetModuleAtIndex(2)
+target.GetModuleAtIndex(1)
 .GetObjectFileHeaderAddress()
 .GetLoadAddress(target)
 )
@@ -140,6 +119,15 @@
 self.assertNotEqual(
 libtwo_uuid, None, "Could not get uuid of built libtwo.dylib"
 )
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % self.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 aout_exe
 shell_cmds = [
@@ -149,27 +137,47 @@
 

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/include/lldb/Target/DynamicLoader.h:267
   ///
+  /// \param[in] allow_use_memory_image_last_resort
+  /// If no better binary image can be found, allow reading the binary

JDevlieghere wrote:
> Nit: seems like either `allow_memory_image_last_resort` or 
> `use_memory_image_last_resort` would convey the same thing. 
Yes, good suggestion, my name wasn't very good here.



Comment at: 
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py:38-39
 )
+if self.TraceOn():
+self.runCmd("script print('Creating corefile with command %s')" % 
cmd)
 call(cmd, shell=True)

bulbazord wrote:
> JDevlieghere wrote:
> > Out of curiosity what's the point of doing `self.runCmd("script print ...` 
> > vs printing the same thing directly? Does this run in another context or 
> > something? Do we save the command output somewhere? 
> I'm also curious. Printing directly is probably less expensive than running a 
> print command through the lldb python script interpreter.
In the past, when I had an SB API test `print()` directly, the trace output is 
buffered differently than STDOUT, so I've done this hack in a few places to get 
the print to happen exactly when the test is executing.  Doing a `if 
self.TraceOn(): self.runCmd("bt"); print("got here")` will not print the two 
next to each other, iirc.  I can switch to a print and confirm my memory of 
this.



Comment at: 
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py:122-123
 )
+dwarfdump_cmd_output = subprocess.check_output(
+('/usr/bin/dwarfdump --uuid "%s"' % self.aout_exe), shell=True
+).decode("utf-8")

JDevlieghere wrote:
> I know this was moved and this goes beyond the scope of this patch, but we 
> should be using the just-built `llvm-dwarfdump` instead of whatever happens 
> to be on the system. Obviously no big deal for extracting the `--uuid` but 
> it's relatively easy to hook up and what we do for other llvm tools such as 
> dsymutil. objdump, etc. 
I wasn't sure how do find & run that in the build dir; it's used in a few Shell 
tests but none in API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

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

[Lldb-commits] [PATCH] D157152: [lldb] Make TSan errors fatal when running the test suite

[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/D157152/new/

https://reviews.llvm.org/D157152

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

[Lldb-commits] [PATCH] D157168: [lldb] [mach-o corefiles] If we have LC_NOTE metadata and can't find a binary, don't fall back to an exhaustive scan

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Patch looks good to me, one small suggestion about naming.




Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.h:89
   void CreateMemoryRegions();
-  void LoadBinariesViaMetadata();
+  bool LoadBinariesViaMetadata();
   void LoadBinariesViaExhaustiveSearch();

Since `LoadBinariesViaMetadata` can fail and we now care about the return 
value, what do you think about the name `TryLoadBinariesViaMetadata`? I think a 
name that indicates that it may fail to actually load would make it easier to 
read at a glance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157168

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

[Lldb-commits] [PATCH] D157167: [lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

I don't have any objections other than the questions Jonas has asked. LGTM 
otherwise




Comment at: 
lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py:38-39
 )
+if self.TraceOn():
+self.runCmd("script print('Creating corefile with command %s')" % 
cmd)
 call(cmd, shell=True)

JDevlieghere wrote:
> Out of curiosity what's the point of doing `self.runCmd("script print ...` vs 
> printing the same thing directly? Does this run in another context or 
> something? Do we save the command output somewhere? 
I'm also curious. Printing directly is probably less expensive than running a 
print command through the lldb python script interpreter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157167

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

[Lldb-commits] [lldb] aa27848 - [lldb] Fix typo in comments and in test

[David Spickett via lldb-commits]

Author: Eymen Ünay
Date: 2023-08-07T12:56:55Z
New Revision: aa2784876a245f10811f65eb748c43574c17a173

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

LOG: [lldb] Fix typo in comments and in test

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2d706835fd2400..56e1726aa5710c 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -2755,8 +2755,8 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
   assert(vm_nlist_bytes_read == nlist_byte_size * nlistCount);
 
   // We don't know the size of the string table. It's cheaper
-  // to map the whol VM region than to determine the size by
-  // parsing all teh nlist entries.
+  // to map the whole VM region than to determine the size by
+  // parsing all the nlist entries.
   vm_address_t string_address = (vm_address_t)stringTable;
   vm_size_t region_size;
   mach_msg_type_number_t info_count = 
VM_REGION_BASIC_INFO_COUNT_64;

diff  --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp 
b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index bfb59eceb2d025..8ff4ae1e19c0f6 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -53,7 +53,7 @@ lldb::ProcessSP ProcessElfCore::CreateInstance(lldb::TargetSP 
target_sp,
bool can_connect) {
   lldb::ProcessSP process_sp;
   if (crash_file && !can_connect) {
-// Read enough data for a ELF32 header or ELF64 header Note: Here we care
+// Read enough data for an ELF32 header or ELF64 header Note: Here we care
 // about e_type field only, so it is safe to ignore possible presence of
 // the header extension.
 const size_t header_size = sizeof(llvm::ELF::Elf64_Ehdr);

diff  --git 
a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py 
b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
index c2b6da11630f37..9e553c57f39cb7 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -348,7 +348,7 @@ def test_flags_unknown_attribute(self):
 
 @skipIfXmlSupportMissing
 @skipIfRemote
-def test_flags_requried_attributes(self):
+def test_flags_required_attributes(self):
 # flags must have an id and size so the flags with "C" is the only 
valid one
 # here.
 self.setup_register_test(

diff  --git 
a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py 
b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
index bca1e5a8403681..2a3d291086b662 100644
--- a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -91,7 +91,7 @@ def test_uuid_modules_with_age(self):
 def test_uuid_modules_elf_build_id_16(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose value is valid and is 16 bytes long.
+and contains an ELF build ID whose value is valid and is 16 bytes long.
 """
 modules = 
self.get_minidump_modules("linux-arm-uuids-elf-build-id-16.yaml")
 self.assertEqual(2, len(modules))
@@ -101,7 +101,7 @@ def test_uuid_modules_elf_build_id_16(self):
 def test_uuid_modules_elf_build_id_20(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose value is valid and is 20 bytes long.
+and contains an ELF build ID whose value is valid and is 20 bytes long.
 """
 modules = 
self.get_minidump_modules("linux-arm-uuids-elf-build-id-20.yaml")
 self.assertEqual(2, len(modules))
@@ -115,7 +115,7 @@ def test_uuid_modules_elf_build_id_20(self):
 def test_uuid_modules_elf_build_id_zero(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose valu

[Lldb-commits] [PATCH] D157214: [lldb] Fix typo in comments and in test

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaa2784876a24: [lldb] Fix typo in comments and in test 
(authored by Eymay, committed by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157214

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py


Index: lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
===
--- lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -91,7 +91,7 @@
 def test_uuid_modules_elf_build_id_16(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose value is valid and is 16 bytes long.
+and contains an ELF build ID whose value is valid and is 16 bytes long.
 """
 modules = 
self.get_minidump_modules("linux-arm-uuids-elf-build-id-16.yaml")
 self.assertEqual(2, len(modules))
@@ -101,7 +101,7 @@
 def test_uuid_modules_elf_build_id_20(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose value is valid and is 20 bytes long.
+and contains an ELF build ID whose value is valid and is 20 bytes long.
 """
 modules = 
self.get_minidump_modules("linux-arm-uuids-elf-build-id-20.yaml")
 self.assertEqual(2, len(modules))
@@ -115,7 +115,7 @@
 def test_uuid_modules_elf_build_id_zero(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-and contains a ELF build ID whose value is all zero.
+and contains an ELF build ID whose value is all zero.
 """
 modules = 
self.get_minidump_modules("linux-arm-uuids-elf-build-id-zero.yaml")
 self.assertEqual(2, len(modules))
@@ -125,7 +125,7 @@
 def test_uuid_modules_elf_build_id_same(self):
 """
 Test multiple modules having a MINIDUMP_MODULE.CvRecord that is
-valid, and contains a ELF build ID whose value is the same. There
+valid, and contains an ELF build ID whose value is the same. There
 is an assert in the PlaceholderObjectFile that was firing when we
 encountered this which was crashing the process that was checking
 if PlaceholderObjectFile.m_base was the same as the address this
Index: lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -348,7 +348,7 @@
 
 @skipIfXmlSupportMissing
 @skipIfRemote
-def test_flags_requried_attributes(self):
+def test_flags_required_attributes(self):
 # flags must have an id and size so the flags with "C" is the only 
valid one
 # here.
 self.setup_register_test(
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -53,7 +53,7 @@
bool can_connect) {
   lldb::ProcessSP process_sp;
   if (crash_file && !can_connect) {
-// Read enough data for a ELF32 header or ELF64 header Note: Here we care
+// Read enough data for an ELF32 header or ELF64 header Note: Here we care
 // about e_type field only, so it is safe to ignore possible presence of
 // the header extension.
 const size_t header_size = sizeof(llvm::ELF::Elf64_Ehdr);
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -2755,8 +2755,8 @@
   assert(vm_nlist_bytes_read == nlist_byte_size * nlistCount);
 
   // We don't know the size of the string table. It's cheaper
-  // to map the whol VM region than to determine the size by
-  // parsing all teh nlist entries.
+  // to map the whole VM region than to determine the size by
+  // parsing all the nlist entries.
   vm_address_t string_address = (vm_address_t)stringTable;
   vm_size_t region_size;
   mach_msg_type_number_t info_count = 
VM_

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

LGTM.


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D157214: [lldb] Fix typo in comments and in test

[Eymen Ünay via Phabricator via lldb-commits]

Eymay added a comment.

Thanks @DavidSpickett. Can you land this patch for me? 
Please use “Eymen Ünay eymenu...@outlook.com" to commit the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157214

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

[Lldb-commits] [lldb] 91a0e83 - [lldb] Make IR interpreter timeout test more loose

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-08-07T09:42:26Z
New Revision: 91a0e832d42abc2890d4f8871a14003de6a9919e

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

LOG: [lldb] Make IR interpreter timeout test more loose

This has failed once in a while on our Windows on Arm bot:
https://lab.llvm.org/buildbot/#/builders/219/builds/4688

Traceback (most recent call last):
  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\...
self.assertGreaterEqual(duration_sec, 1)
AssertionError: 0.9907491207122803 not greater than or equal to 1

We're not here to check that Python/the C++ lib/the OS implemented
timers correctly, so accept anything 0.95 or greater.

Added: 


Modified: 
lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py

Removed: 




diff  --git 
a/lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py 
b/lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
index 1b11d8e5ba054b..5f98bb51a49c1b 100644
--- a/lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
+++ b/lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
@@ -44,7 +44,8 @@ def test_interpreter_timeout(self):
 options.SetTimeoutInMicroSeconds(100)
 res, duration_sec = self.time_expression(inf_loop, options)
 self.assertIn(timeout_error, str(res.GetError()))
-self.assertGreaterEqual(duration_sec, 1)
+# Anything within 5% of 1s is fine, to account for machine 
diff erences.
+self.assertGreaterEqual(duration_sec, 0.95)
 self.assertLess(duration_sec, 30)
 
 def test_interpreter_interrupt(self):



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

[Lldb-commits] [PATCH] D157214: [lldb] Fix typo in comments and in test

[David Spickett via Phabricator via lldb-commits]

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

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157214

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