[Lldb-commits] [lldb] 12c9c4a - [lldb/host] Remove monitor_signals argument from process monitoring functions

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-24T11:12:59+01:00
New Revision: 12c9c4a8853740c41b183495f4d9931e7eee5268

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

LOG: [lldb/host] Remove monitor_signals argument from process monitoring 
functions

All current callers set the argument to false. monitor_signals=true used
to be used in the Process plugins (which needed to know when the
debugged process gets a signal), but this implementation has several
serious issues, which means that individual process plugins now
orchestrate the monitoring of debugged processes themselves.

This allows us to simplify the implementation (no need to play with
process groups), and the interface (we only catch fatal events, so the
callback is always called just once).

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

Added: 


Modified: 
lldb/include/lldb/Host/Host.h
lldb/include/lldb/Host/HostNativeProcessBase.h
lldb/include/lldb/Host/HostProcess.h
lldb/include/lldb/Host/ProcessLaunchInfo.h
lldb/include/lldb/Host/posix/HostProcessPosix.h
lldb/include/lldb/Host/windows/HostProcessWindows.h
lldb/source/Host/common/Host.cpp
lldb/source/Host/common/HostProcess.cpp
lldb/source/Host/common/MonitoringProcessLauncher.cpp
lldb/source/Host/common/ProcessLaunchInfo.cpp
lldb/source/Host/macosx/objcxx/Host.mm
lldb/source/Host/posix/HostProcessPosix.cpp
lldb/source/Host/windows/HostProcessWindows.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/unittests/tools/lldb-server/tests/TestClient.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h
index 1fdf7eab7eb88..b78988721b507 100644
--- a/lldb/include/lldb/Host/Host.h
+++ b/lldb/include/lldb/Host/Host.h
@@ -61,47 +61,30 @@ inline bool operator!=(WaitStatus a, WaitStatus b) { return 
!(a == b); }
 /// Host is a class that answers information about the host operating system.
 class Host {
 public:
-  typedef std::function // Exit value of process if signal is zero
+  typedef std::function // Exit value of process if signal is
+  // zero
   MonitorChildProcessCallback;
 
   /// Start monitoring a child process.
   ///
   /// Allows easy monitoring of child processes. \a callback will be called
-  /// when the child process exits or if it gets a signal. The callback will
-  /// only be called with signals if \a monitor_signals is \b true. \a
-  /// callback will usually be called from another thread so the callback
-  /// function must be thread safe.
-  ///
-  /// When the callback gets called, the return value indicates if monitoring
-  /// should stop. If \b true is returned from \a callback the information
-  /// will be removed. If \b false is returned then monitoring will continue.
-  /// If the child process exits, the monitoring will automatically stop after
-  /// the callback returned regardless of the callback return value.
+  /// when the child process exits or if it dies from a signal.
   ///
   /// \param[in] callback
   /// A function callback to call when a child receives a signal
-  /// (if \a monitor_signals is true) or a child exits.
+  /// or exits.
   ///
   /// \param[in] pid
-  /// The process ID of a child process to monitor, -1 for all
-  /// processes.
-  ///
-  /// \param[in] monitor_signals
-  /// If \b true the callback will get called when the child
-  /// process gets a signal. If \b false, the callback will only
-  /// get called if the child process exits.
+  /// The process ID of a child process to monitor.
   ///
   /// \return
   /// A thread handle that can be used to cancel the thread that
   /// was spawned to monitor \a pid.
-  ///
-  /// \see static void Host::StopMonitoringChildProcess (uint32_t)
   static llvm::Expected
   StartMonitoringChildProcess(const MonitorChildProcessCallback &callback,
-  lldb::pid_t pid, bool monitor_signals);
+  lldb::pid_t pid);
 
   enum SystemLogType { eSystemLogWarning, eSystemLogError };
 

diff  --git a/lldb/include/lldb/Host/HostNativeProcessBase.h 
b/lldb/include/lldb/Host/HostNativeProcessBase.h
index 349e3349183b5..fed29662fab8b 100644
--- a/lldb/include/lldb/Host/HostNativeProcessBase.h
+++ b/lldb/include/lldb/Host/HostNativeProcessBase.h
@@ -37,8 +37,7 @@ class HostNativeProcessB

[Lldb-commits] [PATCH] D120425: [lldb/host] Remove monitor_signals argument from process monitoring functions

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG12c9c4a88537: [lldb/host] Remove monitor_signals argument 
from process monitoring functions (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120425

Files:
  lldb/include/lldb/Host/Host.h
  lldb/include/lldb/Host/HostNativeProcessBase.h
  lldb/include/lldb/Host/HostProcess.h
  lldb/include/lldb/Host/ProcessLaunchInfo.h
  lldb/include/lldb/Host/posix/HostProcessPosix.h
  lldb/include/lldb/Host/windows/HostProcessWindows.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/common/HostProcess.cpp
  lldb/source/Host/common/MonitoringProcessLauncher.cpp
  lldb/source/Host/common/ProcessLaunchInfo.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Host/posix/HostProcessPosix.cpp
  lldb/source/Host/windows/HostProcessWindows.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
  
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/unittests/tools/lldb-server/tests/TestClient.cpp

Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -102,8 +102,7 @@
   // TODO: Use this callback to detect botched launches. If lldb-server does not
   // start, we can print a nice error message here instead of hanging in
   // Accept().
-  Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback,
- false);
+  Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback);
 
   status = Host::LaunchProcess(Info);
   if (status.Fail())
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -344,10 +344,9 @@
 
   lldb::thread_result_t AsyncThread();
 
-  static bool
+  static void
   MonitorDebugserverProcess(std::weak_ptr process_wp,
-lldb::pid_t pid, bool exited, int signo,
-int exit_status);
+lldb::pid_t pid, int signo, int exit_status);
 
   lldb::StateType SetThreadStopInfo(StringExtractor &stop_packet);
 
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
@@ -3366,7 +3366,7 @@
 const std::weak_ptr this_wp =
 std::static_pointer_cast(shared_from_this());
 debugserver_launch_info.SetMonitorProcessCallback(
-std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3, _4), false);
+std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3));
 debugserver_launch_info.SetUserID(process_info.GetUserID());
 
 #if defined(__APPLE__)
@@ -3445,16 +3445,14 @@
   return error;
 }
 
-bool ProcessGDBRemote::MonitorDebugserverProcess(
+void ProcessGDBRemote::MonitorDebugserverProcess(
 std::weak_ptr process_wp, lldb::pid_t debugserver_pid,
-bool exited,// True if the process did exit
 int signo,  // Zero for no signal
 int exit_status // Exit value of process if signal is zero
 ) {
   // "debugserver_pid" argument passed in is the process ID for debugserver
   // that we are tracking...
   Log *log = GetLog(GDBRLog::Process);
-  const bool handled = true;
 
   LLDB_LOGF(log,
 "ProcessGDBRemote::%s(process_wp, pid=%" PRIu64
@@ -3465,7 +3463,7 @@
   LLDB_LOGF(log, "ProcessGDBRemote::%s(process = %p)", __FUNCTION__,
 static_cast(process_sp.get()));
   if (!process_sp || process_sp->m_debugserver_pid != debugserver_pid)
-return handled;
+return;
 
   // Sleep for a half a second to make sure our inferior process has time to
   // set its exit status before we set it incorrectly when both the debugserver
@@ -3499,7 +3497,6 @@
   // Debugserver has exited we need to let our ProcessGDBRemote know that it no
   // longer has a debugserver instance
   process_sp->m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
-  return handled;
 }
 
 void ProcessGDBRemote::KillDebugserverProcess() {
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
+++ lldb/sou

[Lldb-commits] [lldb] c64dbb6 - [lldb] Fix windows build for D120425

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-24T11:50:54+01:00
New Revision: c64dbb66d98d78bfca80ef23959747fe76911a4a

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

LOG: [lldb] Fix windows build for D120425

Added: 


Modified: 
lldb/source/Host/windows/Host.cpp

Removed: 




diff  --git a/lldb/source/Host/windows/Host.cpp 
b/lldb/source/Host/windows/Host.cpp
index a881cfae2151c..df7859d9b46c9 100644
--- a/lldb/source/Host/windows/Host.cpp
+++ b/lldb/source/Host/windows/Host.cpp
@@ -195,8 +195,7 @@ bool Host::GetProcessInfo(lldb::pid_t pid, 
ProcessInstanceInfo &process_info) {
 }
 
 llvm::Expected Host::StartMonitoringChildProcess(
-const Host::MonitorChildProcessCallback &callback, lldb::pid_t pid,
-bool monitor_signals) {
+const Host::MonitorChildProcessCallback &callback, lldb::pid_t pid) {
   return HostThread();
 }
 



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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 

JDevlieghere wrote:
> I see an opportunity for a little RAII helper.
What kind of a helper did you have in mind? Practically the entire function 
consists of setup and teardown in preparation for the `raise(signo)` call. If I 
wanted to be fancy I could put all of that in a helper, but I don't think that 
would make it cleaner. Plus, we also need to be careful about the functions we 
call from a signal handler, and I really don't know whether e.g. 
`llvm::make_scope_exit` is guaranteed to not allocate (heap) memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [lldb] b5eeb88 - [lldb] One more fix for the MonitorChildProcess patch (D120425)

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-24T12:06:42+01:00
New Revision: b5eeb8873af93029221e97ad4d89febb22f62fd3

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

LOG: [lldb] One more fix for the MonitorChildProcess patch (D120425)

Added: 


Modified: 
lldb/source/Host/windows/HostProcessWindows.cpp

Removed: 




diff  --git a/lldb/source/Host/windows/HostProcessWindows.cpp 
b/lldb/source/Host/windows/HostProcessWindows.cpp
index 5462040f6cb71..1955f57bbc383 100644
--- a/lldb/source/Host/windows/HostProcessWindows.cpp
+++ b/lldb/source/Host/windows/HostProcessWindows.cpp
@@ -70,7 +70,7 @@ MonitorThread(const Host::MonitorChildProcessCallback 
&callback,
 
   ::WaitForSingleObject(process_handle, INFINITE);
   ::GetExitCodeProcess(process_handle, &exit_code);
-  callback(::GetProcessId(process_handle), true, 0, exit_code);
+  callback(::GetProcessId(process_handle), 0, exit_code);
   ::CloseHandle(process_handle);
   return {};
 }



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

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

So, it seems we were all correct. The mach-o core file implementation does 
permit writing registers, while the elf one doesn't. I guess we never got that 
feature request for elf.

I don't think that's a blocker for writing this test though. If we decide to 
support writing registers in for elf too, we can always change/remove the test.

Another possibility is to use the gdb-client test infrastructure to simulate a 
server which refuses to write to a register, but those tests are slightly 
tricky to write, so I don't think that's worth it here (unless you're 
interested in getting to know that infra, that is).




Comment at: 
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:151-152
+reg_value: lldb.SBValue = frame.FindRegister('eax')
+if not reg_value.IsValid():
+  return
+

I don't think there's a valid reason for why should this ever fail.


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

https://reviews.llvm.org/D120319

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

[Lldb-commits] [PATCH] D120425: [lldb/host] Remove monitor_signals argument from process monitoring functions

[Nico Weber via Phabricator via lldb-commits]

thakis added a comment.

Breaks Mac build: http://45.33.8.238/macm1/28811/step_4.txt

Ptal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120425

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

[Lldb-commits] [lldb] a85d3b6 - [lldb] Fix macos build for D120425

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-24T12:47:43+01:00
New Revision: a85d3b66cb968a8bdbdfedb67dda0475c81c1c93

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

LOG: [lldb] Fix macos build for D120425

Added: 


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

Removed: 




diff  --git a/lldb/source/Host/macosx/objcxx/Host.mm 
b/lldb/source/Host/macosx/objcxx/Host.mm
index aac6dff7c131..06d14ede35c3 100644
--- a/lldb/source/Host/macosx/objcxx/Host.mm
+++ b/lldb/source/Host/macosx/objcxx/Host.mm
@@ -1474,7 +1474,6 @@ static bool ShouldLaunchUsingXPC(ProcessLaunchInfo 
&launch_info) {
   callback_copy(pid, signal, exit_status);
 
 ::dispatch_source_cancel(source);
-}
   }
 });
 



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

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin updated this revision to Diff 411065.
ilya-nozhkin added a comment.

Replaced redundant check in the test with assert. Also, I've removed type 
annotations to be consistent with the code around.


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

https://reviews.llvm.org/D120319

Files:
  lldb/source/Core/ValueObjectRegister.cpp
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py


Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -133,6 +133,29 @@
 self.assertEqual(len(bytesread), 16)
 self.dbg.DeleteTarget(target)
 
+@skipIfLLVMTargetMissing("X86")
+def test_write_register(self):
+"""Test that writing to register results in an error and that error
+   message is set."""
+target = self.dbg.CreateTarget("linux-x86_64.out")
+process = target.LoadCore("linux-x86_64.core")
+self.assertTrue(process, PROCESS_IS_VALID)
+
+thread = process.GetSelectedThread()
+self.assertTrue(thread)
+
+frame = thread.GetSelectedFrame()
+self.assertTrue(frame)
+
+reg_value = frame.FindRegister('eax')
+self.assertTrue(reg_value)
+
+error = lldb.SBError()
+success = reg_value.SetValueFromCString('10', error)
+self.assertFalse(success)
+self.assertTrue(error.Fail())
+self.assertIsNotNone(error.GetCString())
+
 @skipIfLLVMTargetMissing("X86")
 def test_FPR_SSE(self):
 # check x86_64 core file
Index: lldb/source/Core/ValueObjectRegister.cpp
===
--- lldb/source/Core/ValueObjectRegister.cpp
+++ lldb/source/Core/ValueObjectRegister.cpp
@@ -269,26 +269,30 @@
   // The new value will be in the m_data.  Copy that into our register value.
   error =
   m_reg_value.SetValueFromString(&m_reg_info, llvm::StringRef(value_str));
-  if (error.Success()) {
-if (m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
-  SetNeedsUpdate();
-  return true;
-} else
-  return false;
-  } else
+  if (!error.Success())
 return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
+error.SetErrorString("unable to write back to register");
+return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::SetData(DataExtractor &data, Status &error) {
   error = m_reg_value.SetValueFromData(&m_reg_info, data, 0, false);
-  if (error.Success()) {
-if (m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
-  SetNeedsUpdate();
-  return true;
-} else
-  return false;
-  } else
+  if (!error.Success())
 return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
+error.SetErrorString("unable to write back to register");
+return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::ResolveValue(Scalar &scalar) {


Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -133,6 +133,29 @@
 self.assertEqual(len(bytesread), 16)
 self.dbg.DeleteTarget(target)
 
+@skipIfLLVMTargetMissing("X86")
+def test_write_register(self):
+"""Test that writing to register results in an error and that error
+   message is set."""
+target = self.dbg.CreateTarget("linux-x86_64.out")
+process = target.LoadCore("linux-x86_64.core")
+self.assertTrue(process, PROCESS_IS_VALID)
+
+thread = process.GetSelectedThread()
+self.assertTrue(thread)
+
+frame = thread.GetSelectedFrame()
+self.assertTrue(frame)
+
+reg_value = frame.FindRegister('eax')
+self.assertTrue(reg_value)
+
+error = lldb.SBError()
+success = reg_value.SetValueFromCString('10', error)
+self.assertFalse(success)
+self.assertTrue(error.Fail())
+self.assertIsNotNone(error.GetCString())
+
 @skipIfLLVMTargetMissing("X86")
 def test_FPR_SSE(self):
 # check x86_64 core file
Index: lldb/source/Core/ValueObjectRegister.cpp
===
--- lldb/source/Core/ValueObjectRegister.cpp
+++ lldb/source/Core/ValueObjectRegister.cpp
@@ -269,26 +269,30 @@
   // The new value will be in the m_data.  Copy that into our register value.
   error =
   m_reg_value.SetValueFromString(&m_reg_info, llvm::StringRef(value_str));
-  if (error.Success()) {
-if (m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
-  SetNeedsUpdat

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin added a comment.

In D120319#3342657 , @labath wrote:

> Another possibility is to use the gdb-client test infrastructure to simulate 
> a server which refuses to write to a register

It would be the most proper way, I agree, but I think that it is an overkill 
for such a simple case.


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

https://reviews.llvm.org/D120319

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

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Core/Address.cpp:739
-  s->PutCString(", location = ");
-  var_sp->DumpLocationForAddress(s, *this);
-  s->PutCString(", decl = ");

This place was the only caller of Variable::DumpLocationForAddress. What if we, 
instead of duplicating its logic here, just change the function to do what we 
want?
The interface could be as simple as `Variable::DumpLocations(Stream&, Address)` 
where, if one provides an invalid Address, then all locations get dumped, and 
one can request a specific location by providing a concrete address.
We might even do something similar for the DWARFExpression class, and avoid the 
filter callbacks completely.



Comment at: lldb/source/Expression/DWARFExpression.cpp:2751-2753
   addr_t slide = load_function_start - m_loclist_addresses->func_file_addr;
-  loc->Range->LowPC += slide;
-  loc->Range->HighPC += slide;
+  loc.Range->LowPC += slide;
+  loc.Range->HighPC += slide;

Could the sliding happen inside `GetLocationExpressions`, so that we don't have 
to  repeat it in each callback?



Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:28-29
 # CHECK: Variable{{.*}}, name = "x0", {{.*}}, scope = parameter, location =
 # CHECK-NEXT:  [0x, 0x0001): DW_OP_reg5 RDI
 # CHECK-NEXT:  [0x0001, 0x0006): DW_OP_reg0 RAX
 # CHECK: Variable{{.*}}, name = "x1", {{.*}}, scope = parameter

zequanwu wrote:
> labath wrote:
> > zequanwu wrote:
> > > labath wrote:
> > > > zequanwu wrote:
> > > > > labath wrote:
> > > > > > zequanwu wrote:
> > > > > > > labath wrote:
> > > > > > > > zequanwu wrote:
> > > > > > > > > `image dump symfile` already prints valid ranges for 
> > > > > > > > > variables along with where the value is at each range.
> > > > > > > > Are you sure it does?
> > > > > > > > 
> > > > > > > > I was under the impression that there are two distinct range 
> > > > > > > > concepts being combined here. One is the range list member of 
> > > > > > > > the Variable object (as given by `GetScopeRange` -- that's the 
> > > > > > > > one you're printing now), and the other is the list of ranges 
> > > > > > > > hidden in the DWARFExpression object, which come from the 
> > > > > > > > debug_loc(lists) section (that's the one we've been printing so 
> > > > > > > > far). And that the root cause of the confusion is the very 
> > > > > > > > existence of these two concepts.
> > > > > > > > 
> > > > > > > > If I got it wrong, then do let me know, cause it would make 
> > > > > > > > things a lot simpler if there is only one validity concept to 
> > > > > > > > think about.
> > > > > > > Dwarf plugin is supposed to construct the `m_scope_range` member 
> > > > > > > of an Variable, but it doesn't. `scope_ranges` is empty at 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3468.
> > > > > > >  
> > > > > > > `image dump symfile` dumps the dwarf location list in 
> > > > > > > `m_location` in `Variable`. 
> > > > > > > The dwarf location list has more information than `m_scope_range` 
> > > > > > > as it contains info about where the value is during each range. 
> > > > > > > (e.g. which register the variable lives in). 
> > > > > > > 
> > > > > > > So, I think we need to use similar logic to construct 
> > > > > > > `m_scope_range` when creating `Variable` in dwarf plugin like 
> > > > > > > this 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Expression/DWARFExpression.cpp#L145.
> > > > > > Ok, I see where you're coming from. You're essentially saying that 
> > > > > > the fact that the dwarf plugin does not fill this out is a bug.
> > > > > > 
> > > > > > I don't think that's the case. My interpretation was (and [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Symbol/Variable.cpp#L313
> > > > > >  | this comment]] confirms it) that an empty range here means the 
> > > > > > entire enclosing block. (Also, DWARF was for a long time the only 
> > > > > > symbol file plugin, so what it does is kinda "correct by 
> > > > > > definition").
> > > > > > 
> > > > > > I don't think we want to change that interpretation, as forcing a 
> > > > > > copy of the range in the location list would be wasteful (it would 
> > > > > > be different if this was an interface that one could query, and 
> > > > > > that the dwarf plugin could implement by consulting the location 
> > > > > > list). However, since the dwarf class does not actually make use of 
> > > > > > this functionality (it was [[ https://reviews.llvm.org/D17449 | 
> > > > > > added ]] to support DW_AT_start_scope, then broken at some point, 
> > > > > > and eventually [[ https://reviews.llvm.org/D62302 | removed ]]), we 
> > > > > > do have some freedom in def

[Lldb-commits] [PATCH] D110569: [lldb] [Process/FreeBSD] Rework arm64 register access

[Andrew Turner via Phabricator via lldb-commits]

andrew updated this revision to Diff 48.
andrew added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110569

Files:
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD.h
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h

Index: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
===
--- lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
+++ lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
@@ -63,14 +63,29 @@
   // LLDB's GPR/FPU structs.  However, all fields have matching offsets
   // and sizes, so we do not have to worry about these (and we have
   // a unittest to assert that).
-  std::array m_reg_data;
+  std::array m_reg_data;
+  std::array m_fpreg_data;
 #ifdef LLDB_HAS_FREEBSD_WATCHPOINT
   dbreg m_dbreg;
   bool m_read_dbreg;
 #endif
 
-  Status ReadRegisterSet(uint32_t set);
-  Status WriteRegisterSet(uint32_t set);
+  void *GetGPRBuffer() { return &m_reg_data; }
+  size_t GetGPRBufferSize() { return sizeof(m_reg_data); }
+
+  void *GetFPRBuffer() { return &m_fpreg_data; }
+  size_t GetFPRSize() { return sizeof(m_fpreg_data); }
+
+  bool IsGPR(unsigned reg) const;
+  bool IsFPR(unsigned reg) const;
+
+  Status ReadGPR();
+  Status WriteGPR();
+
+  Status ReadFPR();
+  Status WriteFPR();
+
+  uint32_t CalculateFprOffset(const RegisterInfo *reg_info) const;
 
   llvm::Error ReadHardwareDebugInfo() override;
   llvm::Error WriteHardwareDebugRegs(DREGType hwbType) override;
Index: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
===
--- lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
+++ lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
@@ -24,6 +24,8 @@
 #include 
 // clang-format on
 
+#define REG_CONTEXT_SIZE (GetGPRSize() + GetFPRSize())
+
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_freebsd;
@@ -68,30 +70,43 @@
   return count;
 }
 
-Status NativeRegisterContextFreeBSD_arm64::ReadRegisterSet(uint32_t set) {
-  switch (set) {
-  case RegisterInfoPOSIX_arm64::GPRegSet:
-return NativeProcessFreeBSD::PtraceWrapper(PT_GETREGS, m_thread.GetID(),
-   m_reg_data.data());
-  case RegisterInfoPOSIX_arm64::FPRegSet:
-return NativeProcessFreeBSD::PtraceWrapper(
-PT_GETFPREGS, m_thread.GetID(),
-m_reg_data.data() + sizeof(RegisterInfoPOSIX_arm64::GPR));
-  }
-  llvm_unreachable("NativeRegisterContextFreeBSD_arm64::ReadRegisterSet");
+bool NativeRegisterContextFreeBSD_arm64::IsGPR(unsigned reg) const {
+  if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) ==
+  RegisterInfoPOSIX_arm64::GPRegSet)
+return true;
+  return false;
 }
 
-Status NativeRegisterContextFreeBSD_arm64::WriteRegisterSet(uint32_t set) {
-  switch (set) {
-  case RegisterInfoPOSIX_arm64::GPRegSet:
-return NativeProcessFreeBSD::PtraceWrapper(PT_SETREGS, m_thread.GetID(),
-   m_reg_data.data());
-  case RegisterInfoPOSIX_arm64::FPRegSet:
-return NativeProcessFreeBSD::PtraceWrapper(
-PT_SETFPREGS, m_thread.GetID(),
-m_reg_data.data() + sizeof(RegisterInfoPOSIX_arm64::GPR));
-  }
-  llvm_unreachable("NativeRegisterContextFreeBSD_arm64::WriteRegisterSet");
+bool NativeRegisterContextFreeBSD_arm64::IsFPR(unsigned reg) const {
+  if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) ==
+  RegisterInfoPOSIX_arm64::FPRegSet)
+return true;
+  return false;
+}
+
+Status NativeRegisterContextFreeBSD_arm64::ReadGPR() {
+  return NativeProcessFreeBSD::PtraceWrapper(
+  PT_GETREGS, m_thread.GetID(), m_reg_data.data());
+}
+
+Status NativeRegisterContextFreeBSD_arm64::WriteGPR() {
+  return NativeProcessFreeBSD::PtraceWrapper(
+  PT_SETREGS, m_thread.GetID(), m_reg_data.data());
+}
+
+Status NativeRegisterContextFreeBSD_arm64::ReadFPR() {
+  return NativeProcessFreeBSD::PtraceWrapper(
+  PT_GETFPREGS, m_thread.GetID(), m_fpreg_data.data());
+}
+
+Status NativeRegisterContextFreeBSD_arm64::WriteFPR() {
+  return NativeProcessFreeBSD::PtraceWrapper(
+  PT_SETFPREGS, m_thread.GetID(), m_fpreg_data.data());
+}
+
+uint32_t NativeRegisterContextFreeBSD_arm64::CalculateFprOffset(
+const RegisterInfo *reg_info) const {
+  return reg_info->byte_offset - GetGPRSize();
 }
 
 Status
@@ -111,14 +126,30 @@
? reg_info->name
: "");
 
-  uint32_t set = GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg);
-  error = ReadRegisterSet(set);
-  if (error.Fail())

[Lldb-commits] [PATCH] D120485: [lldb][Process/FreeBSD] Add support for address masks on aarch64

[Andrew Turner via Phabricator via lldb-commits]

andrew created this revision.
andrew added a reviewer: mgorny.
Herald added subscribers: kristof.beyls, krytarowski, arichardson.
andrew requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Read the mask using ptrace with PT_GETREGSET with NT_ARM_ADDR_MASK
for live processes, or from the NT_ARM_ADDR_MASK note in a core
file.

As NT_ARM_ADDR_MASK is on FreeBSD is the same value as NT_ARM_PAC_MASK
and it contains a structure with the same layout and a superset of the
same data use this when reading a core file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120485

Files:
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h

Index: lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
+++ lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
@@ -120,6 +120,7 @@
 };
 
 constexpr RegsetDesc AARCH64_PAC_Desc[] = {
+{llvm::Triple::FreeBSD, llvm::Triple::aarch64, llvm::ELF::NT_ARM_PAC_MASK},
 {llvm::Triple::Linux, llvm::Triple::aarch64, llvm::ELF::NT_ARM_PAC_MASK},
 };
 
Index: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
===
--- lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
+++ lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
@@ -36,8 +36,10 @@
 : public NativeRegisterContextFreeBSD,
   public NativeRegisterContextDBReg_arm64 {
 public:
-  NativeRegisterContextFreeBSD_arm64(const ArchSpec &target_arch,
- NativeThreadProtocol &native_thread);
+  NativeRegisterContextFreeBSD_arm64(
+  const ArchSpec &target_arch,
+  NativeThreadProtocol &native_thread,
+  std::unique_ptr register_info_up);
 
   uint32_t GetRegisterSetCount() const override;
 
@@ -65,6 +67,15 @@
   // a unittest to assert that).
   std::array m_reg_data;
   std::array m_fpreg_data;
+
+  struct arm64_addr_mask {
+uint64_t data_mask;
+uint64_t insn_mask;
+  };
+
+  bool m_addr_mask_is_valid;
+  struct arm64_addr_mask m_addr_mask;
+
 #ifdef LLDB_HAS_FREEBSD_WATCHPOINT
   dbreg m_dbreg;
   bool m_read_dbreg;
@@ -76,16 +87,22 @@
   void *GetFPRBuffer() { return &m_fpreg_data; }
   size_t GetFPRSize() { return sizeof(m_fpreg_data); }
 
+  void *GetAddrMaskBuffer() { return &m_addr_mask; }
+  size_t GetAddrMaskBufferSize() { return sizeof(m_addr_mask); }
+
   bool IsGPR(unsigned reg) const;
   bool IsFPR(unsigned reg) const;
+  bool IsAddrMask(unsigned reg) const;
 
   Status ReadGPR();
   Status WriteGPR();
+  Status ReadAddrMask();
 
   Status ReadFPR();
   Status WriteFPR();
 
   uint32_t CalculateFprOffset(const RegisterInfo *reg_info) const;
+  uint32_t CalculateAddrMaskOffset(const RegisterInfo *reg_info) const;
 
   llvm::Error ReadHardwareDebugInfo() override;
   llvm::Error WriteHardwareDebugRegs(DREGType hwbType) override;
Index: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
===
--- lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
+++ lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
@@ -19,9 +19,11 @@
 #include "Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h"
 
 // clang-format off
+#include 
 #include 
 #include 
 #include 
+#include 
 // clang-format on
 
 #define REG_CONTEXT_SIZE (GetGPRSize() + GetFPRSize())
@@ -33,13 +35,19 @@
 NativeRegisterContextFreeBSD *
 NativeRegisterContextFreeBSD::CreateHostNativeRegisterContextFreeBSD(
 const ArchSpec &target_arch, NativeThreadProtocol &native_thread) {
-  return new NativeRegisterContextFreeBSD_arm64(target_arch, native_thread);
+  Flags opt_regsets;
+  opt_regsets.Set(RegisterInfoPOSIX_arm64::eRegsetMaskPAuth);
+  auto register_info_up =
+  std::make_unique(target_arch, opt_regsets);
+  return new NativeRegisterContextFreeBSD_arm64(target_arch, native_thread,
+  std::move(register_info_up));
 }
 
 NativeRegisterContextFreeBSD_arm64::NativeRegisterContextFreeBSD_arm64(
-const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
-: NativeRegisterContextRegisterInfo(
-  native_thread, new RegisterInfoPOSIX_arm64(target_arch, 0))
+const ArchSpec &target_arch, NativeThreadProtocol &native_thread,
+std::unique_ptr register_info_up)
+: NativeRegisterContextRegisterInfo(native_thread,
+register_info_up.release())
 #ifdef LLDB_HAS_FREEBSD_WATCHPOINT
   ,
   m_read_dbreg(false)
@@ -47,6 +55,9 @@
 {
   ::memset(&m_hwp_regs, 0, sizeof(m_hwp_regs));

[Lldb-commits] [PATCH] D120485: [lldb][Process/FreeBSD] Add support for address masks on aarch64

[Ed Maste via Phabricator via lldb-commits]

emaste added inline comments.



Comment at: 
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp:128
+
+#ifdef NT_ARM_ADDR_MASK
+  if (m_addr_mask_is_valid)

This `#define` is coming from our headers? We need to provide this constant 
within LLVM so this will be available on non-FreeBSD hosts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120485

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

[Lldb-commits] [PATCH] D120292: [lldb] Add lldb.find helper function

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

This is really great, this is going to save me a lot of time.

I wonder if we should take this even further and make it a first class command 
in lldb, similar to `apropos`. (I wouldn't make it part of apropos, because it 
can already be overwhelming.) By making it its own command, we can have the 
same command for all the supported scripting languages. Similar to `script` it 
would look at the current scripting language by default, but you could also 
search other supported language with a flag. I think the implementation could 
remain the same. Every script interpreter plugin could have a `find` method 
that it implements (or doesn't). For the Python case, it could call the find 
command you have here.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120292

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

[Lldb-commits] [PATCH] D120485: [lldb][Process/FreeBSD] Add support for address masks on aarch64

[Ed Maste via Phabricator via lldb-commits]

emaste added a comment.

We need to make sure a test covers this as well, perhaps just enabling 
lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120485

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 

labath wrote:
> JDevlieghere wrote:
> > I see an opportunity for a little RAII helper.
> What kind of a helper did you have in mind? Practically the entire function 
> consists of setup and teardown in preparation for the `raise(signo)` call. If 
> I wanted to be fancy I could put all of that in a helper, but I don't think 
> that would make it cleaner. Plus, we also need to be careful about the 
> functions we call from a signal handler, and I really don't know whether e.g. 
> `llvm::make_scope_exit` is guaranteed to not allocate (heap) memory.
I was only referring to the Save/RestoreInputTerminalState() part of this 
function. Something like:

```
class TerminalStateRAII() {
public:
  TerminalStateRAII(Driver* driver) : driver(m_driver) {
if (m_driver)
  m_driver->GetDebugger().SaveInputTerminalState();
  }

  ~SignalHelper() {
if (m_driver)
  m_driver->GetDebugger().SaveInputTerminalState();
  }

private:
  Driver* m_driver;
};
```

Obviously, this isn't at all important, just something that came to mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120292: [lldb] Add lldb.find helper function

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

It's too bad `script` doesn't have subcommands, because that might have been a 
natural place for such a command. Are there any commands you think this could 
be added? Or would this have to be a new top level command? A new top-level 
command seems unfortunate.

Speaking of apropos, this tool could have an apropos-like mode where it also 
searches docstrings in addition to method/property names. I am not sure how 
useful that would be, but it's worth mentioning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120292

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

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 411203.
zequanwu marked an inline comment as done.
zequanwu added a comment.

Update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119963

Files:
  lldb/include/lldb/Core/Address.h
  lldb/include/lldb/Expression/DWARFExpression.h
  lldb/include/lldb/Symbol/Variable.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Address.cpp
  lldb/source/Expression/DWARFExpression.cpp
  lldb/source/Symbol/Variable.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/DW_AT_loclists_base.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc_and_loclists.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loclists-dwo.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loclists-dwp.s
  lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -14,35 +14,35 @@
 # at the inlined function entry.
 image lookup -v -s break_at_inlined_f_in_main
 # CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_consts +42
-# CHECK: name = "unused2", type = "int", location = 
-# CHECK: name = "partial", type = "int", location = DW_OP_reg4 RSI
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0011, 0x0014) -> DW_OP_consts +42
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
+# CHECK: name = "partial", type = "int", valid ranges = , location = [0x0011, 0x0019) -> DW_OP_reg4 RSI
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
 
 # Show variables outsid of the live range of the 'partial' parameter
 # and verify that the output is as expected.
 image lookup -v -s break_at_inlined_f_in_main_between_printfs
 # CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main_between_printfs
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_reg3 RBX
-# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0014, 0x001e) -> DW_OP_reg3 RBX
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
 # Note: image lookup does not show variables outside of their
 #   location, so |partial| is missing here.
 # CHECK-NOT: partial
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
 
 # Check that we show parameters even if all of them are compiled away.
 image lookup -v -s  break_at_inlined_g_in_main
 # CHECK-LABEL: image lookup -v -s  break_at_inlined_g_in_main
-# CHECK: name = "unused", type = "int", location = 
+# CHECK: name = "unused", type = "int", valid ranges = , location = 
 
 # Check that even the other inlined instance of f displays the correct
 # parameters.
 image lookup -v -s  break_at_inlined_f_in_other
 # CHECK-LABEL: image lookup -v -s  break_at_inlined_f_in_other
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_consts +1
-# CHECK: name = "unused2", type = "int", location = 
-# CHECK: name = "partial", type = "int", location =  DW_OP_consts +2
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0001, 0x000b) -> DW_OP_consts +1
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
+# CHECK: name = "partial", type = "int", valid ranges = , location = [0x0001, 0x0006) -> DW_OP_consts +2
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
@@ -12,7 +12,7 @@
 # CHECK-LABEL: image lookup -v -n F1
 # CHECK: CompileUnit: id = {0x0001}, file = "1.c", language = ""
 # CHECK: Function: {{.*}}, name = "F1", range = [0x0001-0x0002)
-# CHECK: Variable: {{.*}}, name = "x", type = "int", location = DW_OP_reg1 RDX
+# CHECK: Variable: {{.*}}, name = "x

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



Comment at: lldb/source/Core/Address.cpp:739
-  s->PutCString(", location = ");
-  var_sp->DumpLocationForAddress(s, *this);
-  s->PutCString(", decl = ");

labath wrote:
> This place was the only caller of Variable::DumpLocationForAddress. What if 
> we, instead of duplicating its logic here, just change the function to do 
> what we want?
> The interface could be as simple as `Variable::DumpLocations(Stream&, 
> Address)` where, if one provides an invalid Address, then all locations get 
> dumped, and one can request a specific location by providing a concrete 
> address.
> We might even do something similar for the DWARFExpression class, and avoid 
> the filter callbacks completely.
I just moved the filter inside `Variable::DumpLocations`, because we still need 
to pass the filter callback to `DWARFExpression::DumpLocaitons` inside 
`Variable::DumpLocaitons` to choose dumping all ranges + locations or just one 
range location. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119963

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

[Lldb-commits] [PATCH] D120292: [lldb] Add lldb.find helper function

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Could you use `help --scripting` (`help -s`) or maybe `help --api`?  Then you 
could also do `apropos -s` if you ended up adding an apropos feature in the 
script interface, which would be consistent and fairly discoverable.  Then for 
folks that used it all the time they could alias it to something shorter 
(though `h -s` isn't really so bad...)

In D120292#3343566 , @kastiglione 
wrote:

> It's too bad `script` doesn't have subcommands, because that might have been 
> a natural place for such a command. Are there any commands you think this 
> could be added? Or would this have to be a new top level command? A new 
> top-level command seems unfortunate.
>
> Speaking of apropos, this tool could have an apropos-like mode where it also 
> searches docstrings in addition to method/property names. I am not sure how 
> useful that would be, but it's worth mentioning.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120292

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

[Lldb-commits] [PATCH] D120517: PlatformMacOSX should be activated for lldb built to run on an iOS etc device

[Jason Molenda via Phabricator via lldb-commits]

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

In the changes Jonas made in https://reviews.llvm.org/D117340 , a small 
oversight was that PlatformMacOSX (despite the name) is active for any native 
Darwin operating system, where lldb and the target process are running on the 
same system.  This patch uses compile-time checks to return the appropriate 
OSType for the OS lldb is being compiled to, so the "host" platform will 
correctly be selected when lldb & the inferior are both running on that OS.  
And a small change to PlatformMacOSX::GetSupportedArchitectures which adds 
additional recognized triples when running on macOS but not other native Darwin 
systems.

When PlatformMacOSX is compiled on a non-Darwin system where we don't have 
these availability macros, we will continue to return macOS as the OSType, as 
we were before.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120517

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp


Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -137,15 +137,21 @@
   std::vector result;
 #if defined(__arm__) || defined(__arm64__) || defined(__aarch64__)
   // macOS for ARM64 support both native and translated x86_64 processes
-  ARMGetSupportedArchitectures(result, llvm::Triple::MacOSX);
 
-  // We can't use x86GetSupportedArchitectures() because it uses
-  // the system architecture for some of its return values and also
-  // has a 32bits variant.
-  result.push_back(ArchSpec("x86_64-apple-macosx"));
-  result.push_back(ArchSpec("x86_64-apple-ios-macabi"));
-  result.push_back(ArchSpec("arm64-apple-ios-macabi"));
-  result.push_back(ArchSpec("arm64e-apple-ios-macabi"));
+  // When cmdline lldb is run on iOS, watchOS, etc, it is still
+  // using "PlatformMacOSX".
+  llvm::Triple::OSType host_os = GetHostOSType();
+  ARMGetSupportedArchitectures(result, host_os);
+
+  if (host_os == llvm::Triple::MacOSX) {
+// We can't use x86GetSupportedArchitectures() because it uses
+// the system architecture for some of its return values and also
+// has a 32bits variant.
+result.push_back(ArchSpec("x86_64-apple-macosx"));
+result.push_back(ArchSpec("x86_64-apple-ios-macabi"));
+result.push_back(ArchSpec("arm64-apple-ios-macabi"));
+result.push_back(ArchSpec("arm64e-apple-ios-macabi"));
+  }
 #else
   x86GetSupportedArchitectures(result);
   result.push_back(ArchSpec("x86_64-apple-ios-macabi"));
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
@@ -174,6 +174,9 @@
   static std::string FindComponentInPath(llvm::StringRef path,
  llvm::StringRef component);
 
+  // The OSType where lldb is running.
+  static llvm::Triple::OSType GetHostOSType();
+
   std::string m_developer_directory;
   llvm::StringMap m_sdk_path;
   std::mutex m_sdk_path_mutex;
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1328,3 +1328,23 @@
 return FileSpec(FindComponentInPath(fspec.GetPath(), "CommandLineTools"));
   return {};
 }
+
+llvm::Triple::OSType PlatformDarwin::GetHostOSType() {
+#if !defined(__APPLE__)
+  return llvm::Triple::MacOSX;
+#else
+#if TARGET_OS_OSX
+  return llvm::Triple::MacOSX;
+#elif TARGET_OS_IOS
+  return llvm::Triple::IOS;
+#elif TARGET_OS_WATCH
+  return llvm::Triple::WatchOS;
+#elif TARGET_OS_TV
+  return llvm::Triple::TvOS;
+#elif TARGET_OS_BRIDGE
+  return llvm::Triple::BridgeOS;
+#else
+#error "LLDB being compiled for an unrecognized Darwin OS"
+#endif
+#endif // __APPLE__
+}


Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -137,15 +137,21 @@
   std::vector result;
 #if defined(__arm__) || defined(__arm64__) || defined(__aarch64__)
   // macOS for ARM64 support both native and translated x86_64 processes
-  ARMGetSupportedArchitectures(result, llvm::Triple::MacOSX);
 
-  // We can't use x86GetSupportedArchitectures() because it uses
-  // the system architecture for some of its return values and also
-  // has a 32bi

[Lldb-commits] [PATCH] D120517: PlatformMacOSX should be activated for lldb built to run on an iOS etc device

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

https://reviews.llvm.org/D120517

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Andreu Carminati via Phabricator via lldb-commits]

andcarminati added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 

JDevlieghere wrote:
> labath wrote:
> > JDevlieghere wrote:
> > > I see an opportunity for a little RAII helper.
> > What kind of a helper did you have in mind? Practically the entire function 
> > consists of setup and teardown in preparation for the `raise(signo)` call. 
> > If I wanted to be fancy I could put all of that in a helper, but I don't 
> > think that would make it cleaner. Plus, we also need to be careful about 
> > the functions we call from a signal handler, and I really don't know 
> > whether e.g. `llvm::make_scope_exit` is guaranteed to not allocate (heap) 
> > memory.
> I was only referring to the Save/RestoreInputTerminalState() part of this 
> function. Something like:
> 
> ```
> class TerminalStateRAII() {
> public:
>   TerminalStateRAII(Driver* driver) : driver(m_driver) {
> if (m_driver)
>   m_driver->GetDebugger().SaveInputTerminalState();
>   }
> 
>   ~SignalHelper() {
> if (m_driver)
>   m_driver->GetDebugger().SaveInputTerminalState();
>   }
> 
> private:
>   Driver* m_driver;
> };
> ```
> 
> Obviously, this isn't at all important, just something that came to mind.
I think this is a good idea to reduce code duplication. Another approach:

```
class TerminalStateRAII() {
public:
  TerminalStateRAII(Driver* driver) : driver(m_driver) {
SaveInputTerminalState();
  }

  ~TerminalStateRAII() {
SaveInputTerminalState();
  }

private:
  Driver* m_driver;
  void SaveInputTerminalState(){
if (m_driver)
  m_driver->GetDebugger().SaveInputTerminalState();
  }
};

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 

andcarminati wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > JDevlieghere wrote:
> > > > I see an opportunity for a little RAII helper.
> > > What kind of a helper did you have in mind? Practically the entire 
> > > function consists of setup and teardown in preparation for the 
> > > `raise(signo)` call. If I wanted to be fancy I could put all of that in a 
> > > helper, but I don't think that would make it cleaner. Plus, we also need 
> > > to be careful about the functions we call from a signal handler, and I 
> > > really don't know whether e.g. `llvm::make_scope_exit` is guaranteed to 
> > > not allocate (heap) memory.
> > I was only referring to the Save/RestoreInputTerminalState() part of this 
> > function. Something like:
> > 
> > ```
> > class TerminalStateRAII() {
> > public:
> >   TerminalStateRAII(Driver* driver) : driver(m_driver) {
> > if (m_driver)
> >   m_driver->GetDebugger().SaveInputTerminalState();
> >   }
> > 
> >   ~SignalHelper() {
> > if (m_driver)
> >   m_driver->GetDebugger().SaveInputTerminalState();
> >   }
> > 
> > private:
> >   Driver* m_driver;
> > };
> > ```
> > 
> > Obviously, this isn't at all important, just something that came to mind.
> I think this is a good idea to reduce code duplication. Another approach:
> 
> ```
> class TerminalStateRAII() {
> public:
>   TerminalStateRAII(Driver* driver) : driver(m_driver) {
> SaveInputTerminalState();
>   }
> 
>   ~TerminalStateRAII() {
> SaveInputTerminalState();
>   }
> 
> private:
>   Driver* m_driver;
>   void SaveInputTerminalState(){
> if (m_driver)
>   m_driver->GetDebugger().SaveInputTerminalState();
>   }
> };
> 
> ```
That's a typo on my part, the destructor needs to call 
`RestoreInputTerminalState` (as opposed to `SaveInputTerminalState`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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