[Lldb-commits] [PATCH] D120100: [lldb/bindings] Expose the progress reporting machinery to the SWIG interface

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/bindings/interface/SBDebugger.i:126-129
+%apply uint64_t& INOUT { uint64_t& progress_id };
+%apply uint64_t& INOUT { uint64_t& completed };
+%apply uint64_t& INOUT { uint64_t& total };
+%apply bool& INOUT { bool& is_debugger_specific };

mib wrote:
> labath wrote:
> > mib wrote:
> > > @labath In my understanding, these tell SWIG to return return the 
> > > reference values as a tuple. They are pre-pending the function return 
> > > value. I'll write a follow-up patch to test it.
> > Ah, that's cool. So it takes the `in` value as a regular argument, and 
> > provides the `out` value in the return value.
> > 
> > I don't suppose there's any chance of making this an `out`-only argument, 
> > to avoid the dummy input argument ?
> I don't think SWIG offers such thing unfortunately.
What about [[ http://www.swig.org/Doc1.3/Arguments.html#Arguments_nn5 | this ]] 
?

> The following typemap rules tell SWIG that pointer is the output value of a 
> function. When used, you do not need to supply the argument when calling the 
> function. <...>

The article speaks about pointer arguments, but in my (simple) test, it seemed 
to work fine for references as well...



Comment at: 
lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py:42
+
+@skipUnlessDarwin
+def test_dwarf_symbol_loading_progress_report(self):

mib wrote:
> labath wrote:
> > Why?
> So far, I know that Linux and macOS report some basic progress events when 
> reading DWARF, but I'm not sure other platforms do, so I might be able to 
> only skip windows.
The original progress patch (D97739) has a test running on linux, so I'd expect 
this should work there too. The test in that patch is disabled on windows, but 
that might be due to some vscode issues. I don't see why there should be any 
differences between windows and linux in terms of the dwarf events generated -- 
both of them don't use accelerator tables by default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120100

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

[Lldb-commits] [PATCH] D119548: [lldb] Fix race condition between lldb-vscode and stop hooks executor

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa2c267e0c9d9: [lldb] Fix race condition between lldb-vscode 
and stop hooks executor (authored by ilya-nozhkin, committed by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119548

Files:
  lldb/include/lldb/Target/Process.h
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
  lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
  lldb/test/API/tools/lldb-vscode/stop-hooks/main.c

Index: lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
@@ -0,0 +1,35 @@
+"""
+Test stop hooks
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbvscode_testcase
+
+
+class TestVSCode_stop_hooks(lldbvscode_testcase.VSCodeTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfRemote
+def test_stop_hooks_before_run(self):
+'''
+Test that there is no race condition between lldb-vscode and
+stop hooks executor
+'''
+program = self.getBuildArtifact("a.out")
+preRunCommands = ['target stop-hook add -o help']
+self.build_and_launch(program, stopOnEntry=True, preRunCommands=preRunCommands)
+
+# The first stop is on entry.
+self.continue_to_next_stop()
+
+breakpoint_ids = self.set_function_breakpoints(['main'])
+# This request hangs if the race happens, because, in that case, the
+# command interpreter is in synchronous mode while lldb-vscode expects
+# it to be in asynchronous mode, so, the process doesn't send the stop
+# event to "lldb.Debugger" listener (which is monitored by lldb-vscode).
+self.continue_to_breakpoints(breakpoint_ids)
+
+self.continue_to_exit()
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3026,6 +3026,14 @@
   if (!launch_info.GetArchitecture().IsValid())
 launch_info.GetArchitecture() = GetArchitecture();
 
+  // Hijacking events of the process to be created to be sure that all events
+  // until the first stop are intercepted (in case if platform doesn't define
+  // its own hijacking listener or if the process is created by the target
+  // manually, without the platform).
+  if (!launch_info.GetHijackListener())
+launch_info.SetHijackListener(
+Listener::MakeListener("lldb.Target.Launch.hijack"));
+
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
@@ -3057,8 +3065,10 @@
 }
 
 // Since we didn't have a platform launch the process, launch it here.
-if (m_process_sp)
+if (m_process_sp) {
+  m_process_sp->HijackProcessEvents(launch_info.GetHijackListener());
   error = m_process_sp->Launch(launch_info);
+}
   }
 
   if (!m_process_sp && error.Success())
@@ -3067,35 +3077,35 @@
   if (!error.Success())
 return error;
 
-  auto at_exit =
-  llvm::make_scope_exit([&]() { m_process_sp->RestoreProcessEvents(); });
+  bool rebroadcast_first_stop =
+  !synchronous_execution &&
+  launch_info.GetFlags().Test(eLaunchFlagStopAtEntry);
 
-  if (!synchronous_execution &&
-  launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
-return error;
+  assert(launch_info.GetHijackListener());
+
+  EventSP first_stop_event_sp;
+  state = m_process_sp->WaitForProcessToStop(llvm::None, &first_stop_event_sp,
+ rebroadcast_first_stop,
+ launch_info.GetHijackListener());
+  m_process_sp->RestoreProcessEvents();
 
-  Li

[Lldb-commits] [lldb] a2c267e - [lldb] Fix race condition between lldb-vscode and stop hooks executor

[Pavel Labath via lldb-commits]

Author: Ilya Nozhkin
Date: 2022-02-22T12:53:55+01:00
New Revision: a2c267e0c9d9b9963f4022caf455327a7d96dfbf

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

LOG: [lldb] Fix race condition between lldb-vscode and stop hooks executor

The race is between these two pieces of code that are executed in two separate
lldb-vscode threads (the first is in the main thread and another is in the
event-handling thread):

```
// lldb-vscode.cpp
g_vsc.debugger.SetAsync(false);
g_vsc.target.Launch(launch_info, error);
g_vsc.debugger.SetAsync(true);
```

```
// Target.cpp
bool old_async = debugger.GetAsyncExecution();
debugger.SetAsyncExecution(true);
debugger.GetCommandInterpreter().HandleCommands(GetCommands(), exc_ctx,
options, result);
debugger.SetAsyncExecution(old_async);
```

The sequence that leads to the bug is this one:
1. Main thread enables synchronous mode and launches the process.
2. When the process is launched, it generates the first stop event.
3. This stop event is catched by the event-handling thread and DoOnRemoval
   is invoked.
4. Inside DoOnRemoval, this thread runs stop hooks. And before running stop
   hooks, the current synchronization mode is stored into old_async (and
   right now it is equal to "false").
5. The main thread finishes the launch and returns to lldb-vscode, the
   synchronization mode is restored to asynchronous by lldb-vscode.
6. Event-handling thread finishes stop hooks processing and restores the
   synchronization mode according to old_async (i.e. makes the mode synchronous)
7. And now the mode is synchronous while lldb-vscode expects it to be
   asynchronous. Synchronous mode forbids the process to broadcast public stop
   events, so, VS Code just hangs because lldb-vscode doesn't notify it about
   stops.

So, this diff makes the target intercept the first stop event if the process is
launched in the synchronous mode, thus preventing stop hooks execution.

The bug is only present on Windows because other platforms already
intercept this event using their own hijacking listeners.

So, this diff also fixes some problems with lldb-vscode tests on Windows to make
it possible to run the related test. Other tests still can't be enabled because
the debugged program prints something into stdout and LLDB can't intercept this
output and redirect it to lldb-vscode properly.

Reviewed By: jingham

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

Added: 
lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
lldb/test/API/tools/lldb-vscode/stop-hooks/main.c

Modified: 
lldb/include/lldb/Target/Process.h
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 7911dac40b705..adceec619ff04 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -3073,6 +3073,9 @@ void PruneThreadPlans();
 
   void ControlPrivateStateThread(uint32_t signal);
 
+  Status LaunchPrivate(ProcessLaunchInfo &launch_info, lldb::StateType &state,
+   lldb::EventSP &event_sp);
+
   Process(const Process &) = delete;
   const Process &operator=(const Process &) = delete;
 };

diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 950dd41666fd6..98b0922e9cfe2 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -231,6 +231,11 @@ def pointer_size():
 
 def is_exe(fpath):
 """Returns true if fpath is an executable."""
+if fpath == None:
+return False
+if sys.platform == 'win32':
+if not fpath.endswith(".exe"):
+fpath += ".exe"
 return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 

diff  --git 
a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
index 255a4805a9737..b0fb17ffa9719 100644
--- 
a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -11,8 +11,8 @@ class VSCodeTestCaseBase(TestBase):
 
 def create_debug_ad

[Lldb-commits] [PATCH] D119548: [lldb] Fix race condition between lldb-vscode and stop hooks executor

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Committed. Thanks for the great patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119548

___
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 created this revision.
ilya-nozhkin added reviewers: labath, clayborg.
Herald added a subscriber: pengfei.
ilya-nozhkin requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

`SetValueFromCString` and `SetData` methods return false if register can't
be written but they don't set a error message. It sometimes confuses callers of
these methods because they try to get the error message in case of failure but
`Status::AsCString` returns `nullptr`.

For example, `lldb-vscode` crashes due to this bug if some register can't be
written. It invokes `SBError::GetCString` in case of error and doesn't check
whether the result is `nullptr` (see `request_setVariable` implementation in
`lldb-vscode.cpp` for more info).

I couldn't write any test for this issue on x86 architecture because all x86
registers are writable. It might be possible to add such test for some other
testable architecture but I don't know much about them and I don't have a
machine with any architecture other than x86 to actually run the test.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120319

Files:
  lldb/source/Core/ValueObjectRegister.cpp


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/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) {
___
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 created this revision.
labath added reviewers: mgorny, DavidSpickett.
labath requested review of this revision.
Herald added a project: LLDB.

Our SIGTSTP handler was working, but that was mostly accidental.

The reason it worked is because lldb is multithreaded for most of its
lifetime and the OS is reasonably fast at responding to signals. So,
what happened was that the kill(SIGTSTP) which we sent from inside the
handler was delivered to another thread while the handler was still set
to SIG_DFL (which then correctly put the entire process to sleep).

Sometimes it happened that the other thread got the second signal after
the first thread had already restored the handler, in which case the
signal handler would run again, and it would again attempt to send the
SIGTSTP signal back to itself.

Normally it didn't take many iterations for the signal to be delivered
quickly enough. However, if you were unlucky (or were playing around
with pexpect) you could get SIGTSTP while lldb was single-threaded, and
in that case, lldb would go into an endless loop because the second
SIGTSTP could only be handled on the main thread, and only after the
handler for the first signal returned (and re-installed itself). In that
situation the handler would keep re-sending the signal to itself.

This patch fixes the issue by implementing the handler the way it
supposed to be done:

- before sending the second SIGTSTP, we unblock the signal (it gets 
automatically blocked upon entering the handler)
- we use raise to send the signal, which makes sure it gets delivered to the 
thread which is running the handler

This also means we don't need the SIGCONT handler, as our TSTP handler
resumes right after the entire process is continued, and we can do the
required work there.

I also include a test case for the SIGTSTP flow. It uses pexpect, but it
includes a couple of extra twists. Specifically, I needed to create an
extra process on top of lldb, which will run lldb in a separate process
group and simulate the role of the shell. This is needed because SIGTSTP
is not effective on a session leader (the signal gets delivered, but it
does not cause a stop) -- normally there isn't anyone to notice the
stop.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120320

Files:
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/test/API/driver/job_control/TestJobControl.py
  lldb/test/API/driver/job_control/shell.py
  lldb/tools/driver/Driver.cpp

Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -668,23 +668,30 @@
   _exit(signo);
 }
 
+#ifndef _WIN32
 void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 
+  // Unblock the signal and remove our handler.
+  sigset_t set;
+  sigemptyset(&set);
+  sigaddset(&set, signo);
+  pthread_sigmask(SIG_UNBLOCK, &set, nullptr);
   signal(signo, SIG_DFL);
-  kill(getpid(), signo);
+
+  // Now re-raise the signal. We will immediately suspend...
+  raise(signo);
+  // ... and resume after a SIGCONT.
+
+  // Now undo the modifications.
+  pthread_sigmask(SIG_BLOCK, &set, nullptr);
   signal(signo, sigtstp_handler);
-}
 
-void sigcont_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().RestoreInputTerminalState();
-
-  signal(signo, SIG_DFL);
-  kill(getpid(), signo);
-  signal(signo, sigcont_handler);
 }
+#endif
 
 void reproducer_handler(void *finalize_cmd) {
   if (SBReproducer::Generate()) {
@@ -831,7 +838,6 @@
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif
 
   int exit_code = 0;
Index: lldb/test/API/driver/job_control/shell.py
===
--- /dev/null
+++ lldb/test/API/driver/job_control/shell.py
@@ -0,0 +1,26 @@
+"""
+Launch a process (given through argv) similar to how a shell would do it.
+"""
+
+import signal
+import subprocess
+import sys
+import os
+
+def preexec_fn():
+orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])
+os.setpgid(0, 0)
+fd = os.open("/dev/tty", os.O_RDONLY)
+os.tcsetpgrp(fd, os.getpgid(0))
+os.close(fd)
+signal.pthread_sigmask(signal.SIG_SETMASK, orig_mask)
+
+if __name__ == "__main__":
+child = subprocess.Popen(sys.argv[1:], preexec_fn=preexec_fn)
+print("PID=%d" % child.pid)
+
+_, status = os.waitpid(child.pid, os.WUNTRACED)
+print("STATUS=%d" % status)
+
+returncode = child.wait()
+print("RETURNCODE=%d" % returncode)
Index: lldb/test/API/driver/job_control/TestJobControl.py
===
--- /dev/null
+++ lldb/test/API/driver/job_control/TestJobControl.py
@@ -0,0 +1,36 @@
+"""
+Test lldb's handling of job control signals (SIGTSTP, SIGCONT).
+"""
+
+
+import lldb

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added a reviewer: JDevlieghere.
Herald added a subscriber: mgorny.
labath requested review of this revision.
Herald added a project: LLDB.

Accept a function object instead of a raw pointer. This avoids a bunch
of boilerplate typically needed to pass arguments to the thread
functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120321

Files:
  lldb/include/lldb/Core/Communication.h
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Host/ThreadLauncher.h
  lldb/include/lldb/Target/Process.h
  lldb/source/API/SBHostOS.cpp
  lldb/source/Core/Communication.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/common/HostNativeThreadBase.cpp
  lldb/source/Host/common/ThreadLauncher.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
  lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/ThreadLauncherTest.cpp

Index: lldb/unittests/Host/ThreadLauncherTest.cpp
===
--- /dev/null
+++ lldb/unittests/Host/ThreadLauncherTest.cpp
@@ -0,0 +1,29 @@
+//===-- ThreadLauncherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/ThreadLauncher.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace lldb_private;
+
+TEST(ThreadLauncherTest, LaunchThread) {
+  std::promise promise;
+  std::future future = promise.get_future();
+  llvm::Expected thread =
+  ThreadLauncher::LaunchThread("test", [&promise] {
+promise.set_value(47);
+return (lldb::thread_result_t)47;
+  });
+  ASSERT_THAT_EXPECTED(thread, llvm::Succeeded());
+  EXPECT_EQ(future.get(), 47);
+  lldb::thread_result_t result;
+  thread->Join(&result);
+  EXPECT_EQ(result, (lldb::thread_result_t)47);
+}
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -12,6 +12,7 @@
   SocketAddressTest.cpp
   SocketTest.cpp
   SocketTestUtilities.cpp
+  ThreadLauncherTest.cpp
   XMLTest.cpp
 )
 
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3509,12 +3509,13 @@
"", GetID());
   }
 
-  // Create the private state thread, and start it running.
-  PrivateStateThreadArgs *args_ptr =
-  new PrivateStateThreadArgs(this, is_secondary_thread);
   llvm::Expected private_state_thread =
-  ThreadLauncher::LaunchThread(thread_name, Process::PrivateStateThread,
-   (void *)args_ptr, 8 * 1024 * 1024);
+  ThreadLauncher::LaunchThread(
+  thread_name,
+  [this, is_secondary_thread] {
+return RunPrivateStateThread(is_secondary_thread);
+  },
+  8 * 1024 * 1024);
   if (!private_state_thread) {
 LLDB_LOG(GetLog(LLDBLog::Host), "failed to launch host thread: {}",
  llvm::toString(private_state_thread.takeError()));
@@ -3729,14 +3730,6 @@
   return error;
 }
 
-thread_result_t Process::PrivateStateThread(void *arg) {
-  std::unique_ptr args_up(
-  static_cast(arg));
-  thread_result_t result =
-  args_up->process->RunPrivateStateThread(args_up->is_secondary_thread);
-  return result;
-}
-
 thread_result_t Process::RunPrivateStateThread(bool is_secondary_thread) {
   bool control_only = true;
 
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
@@ -342,7 +342,7 @@
 
   void StopAsyncThread();
 
-  static lldb::thread_result_t AsyncThread(void *arg);
+  lldb::thread_result_t AsyncThread();
 
   static bool
   MonitorDebugserverProcess(std::weak_ptr process_wp,
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBR

[Lldb-commits] [PATCH] D120322: [lldb] Simplify HostThreadMacOSX

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: JDevlieghere, jingham.
labath requested review of this revision.
Herald added a project: LLDB.

The class is using an incredibly elaborate setup to create and destroy
an NSAutoreleasePool object. We can do it in a much simpler way by
making those calls inside our thread startup function.

The only effect of this patch is that the pool gets released at the end
of the ThreadCreateTrampoline function, instead of slightly later, when
pthreads begin thread-specific cleanup. However, the key destruction
order is unspecified, so nothing should be relying on that.

I didn't find a specific reason for why this would have to be done that
way in git history. It seems that before D5198 
, this was thread-specific
keys were the only way an os implementation (in Host::ThreadCreated)
could attach some value to a thread.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120322

Files:
  lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
  lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm


Index: lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
@@ -7,62 +7,15 @@
 
//===--===//
 
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
-#include "lldb/Host/Host.h"
-
 #include 
 #include 
 
-#include 
-
 using namespace lldb_private;
 
-
-static pthread_once_t g_thread_create_once = PTHREAD_ONCE_INIT;
-static pthread_key_t g_thread_create_key = 0;
-
-namespace {
-class MacOSXDarwinThread {
-public:
-  MacOSXDarwinThread() { m_pool = [[NSAutoreleasePool alloc] init]; }
-
-  ~MacOSXDarwinThread() {
-if (m_pool) {
-  [m_pool drain];
-  m_pool = nil;
-}
-  }
-
-  static void PThreadDestructor(void *v) {
-if (v)
-  delete static_cast(v);
-::pthread_setspecific(g_thread_create_key, NULL);
-  }
-
-protected:
-  NSAutoreleasePool *m_pool = nil;
-
-private:
-  MacOSXDarwinThread(const MacOSXDarwinThread &) = delete;
-  const MacOSXDarwinThread &operator=(const MacOSXDarwinThread &) = delete;
-};
-} // namespace
-
-static void InitThreadCreated() {
-  ::pthread_key_create(&g_thread_create_key,
-   MacOSXDarwinThread::PThreadDestructor);
-}
-
-HostThreadMacOSX::HostThreadMacOSX() : HostThreadPosix() {}
-
-HostThreadMacOSX::HostThreadMacOSX(lldb::thread_t thread)
-: HostThreadPosix(thread) {}
-
 lldb::thread_result_t
 HostThreadMacOSX::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ::pthread_once(&g_thread_create_once, InitThreadCreated);
-  if (g_thread_create_key) {
-::pthread_setspecific(g_thread_create_key, new MacOSXDarwinThread());
-  }
-
-  return HostThreadPosix::ThreadCreateTrampoline(arg);
+  NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
+  lldb::thread_result_t result = HostThreadPosix::ThreadCreateTrampoline(arg);
+  [pool drain];
+  return result;
 }
Index: lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
@@ -17,8 +17,7 @@
   friend class ThreadLauncher;
 
 public:
-  HostThreadMacOSX();
-  HostThreadMacOSX(lldb::thread_t thread);
+  using HostThreadPosix::HostThreadPosix;
 
 protected:
   static lldb::thread_result_t ThreadCreateTrampoline(lldb::thread_arg_t arg);


Index: lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
@@ -7,62 +7,15 @@
 //===--===//
 
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
-#include "lldb/Host/Host.h"
-
 #include 
 #include 
 
-#include 
-
 using namespace lldb_private;
 
-
-static pthread_once_t g_thread_create_once = PTHREAD_ONCE_INIT;
-static pthread_key_t g_thread_create_key = 0;
-
-namespace {
-class MacOSXDarwinThread {
-public:
-  MacOSXDarwinThread() { m_pool = [[NSAutoreleasePool alloc] init]; }
-
-  ~MacOSXDarwinThread() {
-if (m_pool) {
-  [m_pool drain];
-  m_pool = nil;
-}
-  }
-
-  static void PThreadDestructor(void *v) {
-if (v)
-  delete static_cast(v);
-::pthread_setspecific(g_thread_create_key, NULL);
-  }
-
-protected:
-  NSAutoreleasePool *m_pool = nil;
-
-private:
-  MacOSXDarwinThread(const MacOSXDarwinThread &) = delete;
-  const MacOSXDarwinThread &operator=(const MacOSXDarwinThread &) = delete;
-};
-} // namespace
-
-static void InitThreadCreated() {
-  ::pthread_key_create(&g_thread_create_key,
-   MacOSXDarwinThread::PThreadDestructor);
-}
-
-HostThreadMacOSX::HostThreadMacOSX() : HostThreadPosix()

[Lldb-commits] [lldb] 126a260 - [lldb] Remove HostProcess:GetMainModule

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-22T16:00:58+01:00
New Revision: 126a2607a8458c7928aa1da880d45c36560017a6

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

LOG: [lldb] Remove HostProcess:GetMainModule

the function is unused, and the posix implementation is only really correct on 
linux.

Added: 


Modified: 
lldb/include/lldb/Host/HostNativeProcessBase.h
lldb/include/lldb/Host/HostProcess.h
lldb/include/lldb/Host/posix/HostProcessPosix.h
lldb/include/lldb/Host/windows/HostProcessWindows.h
lldb/source/Host/common/HostProcess.cpp
lldb/source/Host/posix/HostProcessPosix.cpp
lldb/source/Host/windows/HostProcessWindows.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/HostNativeProcessBase.h 
b/lldb/include/lldb/Host/HostNativeProcessBase.h
index 5469f8a50e263..349e3349183b5 100644
--- a/lldb/include/lldb/Host/HostNativeProcessBase.h
+++ b/lldb/include/lldb/Host/HostNativeProcessBase.h
@@ -30,7 +30,6 @@ class HostNativeProcessBase {
   virtual ~HostNativeProcessBase() = default;
 
   virtual Status Terminate() = 0;
-  virtual Status GetMainModule(FileSpec &file_spec) const = 0;
 
   virtual lldb::pid_t GetProcessId() const = 0;
   virtual bool IsRunning() const = 0;

diff  --git a/lldb/include/lldb/Host/HostProcess.h 
b/lldb/include/lldb/Host/HostProcess.h
index 0b7c303642239..00cb6a212736c 100644
--- a/lldb/include/lldb/Host/HostProcess.h
+++ b/lldb/include/lldb/Host/HostProcess.h
@@ -37,7 +37,6 @@ class HostProcess {
   ~HostProcess();
 
   Status Terminate();
-  Status GetMainModule(FileSpec &file_spec) const;
 
   lldb::pid_t GetProcessId() const;
   bool IsRunning() const;

diff  --git a/lldb/include/lldb/Host/posix/HostProcessPosix.h 
b/lldb/include/lldb/Host/posix/HostProcessPosix.h
index 5def1b77eefe6..eec19b621a890 100644
--- a/lldb/include/lldb/Host/posix/HostProcessPosix.h
+++ b/lldb/include/lldb/Host/posix/HostProcessPosix.h
@@ -27,7 +27,6 @@ class HostProcessPosix : public HostNativeProcessBase {
   static Status Signal(lldb::process_t process, int signo);
 
   Status Terminate() override;
-  Status GetMainModule(FileSpec &file_spec) const override;
 
   lldb::pid_t GetProcessId() const override;
   bool IsRunning() const override;

diff  --git a/lldb/include/lldb/Host/windows/HostProcessWindows.h 
b/lldb/include/lldb/Host/windows/HostProcessWindows.h
index 925d565c275ef..dc27bdc46bb8f 100644
--- a/lldb/include/lldb/Host/windows/HostProcessWindows.h
+++ b/lldb/include/lldb/Host/windows/HostProcessWindows.h
@@ -25,7 +25,6 @@ class HostProcessWindows : public HostNativeProcessBase {
   void SetOwnsHandle(bool owns);
 
   Status Terminate() override;
-  Status GetMainModule(FileSpec &file_spec) const override;
 
   lldb::pid_t GetProcessId() const override;
   bool IsRunning() const override;

diff  --git a/lldb/source/Host/common/HostProcess.cpp 
b/lldb/source/Host/common/HostProcess.cpp
index 06dd192013ba4..83b856df36eb9 100644
--- a/lldb/source/Host/common/HostProcess.cpp
+++ b/lldb/source/Host/common/HostProcess.cpp
@@ -22,10 +22,6 @@ HostProcess::~HostProcess() = default;
 
 Status HostProcess::Terminate() { return m_native_process->Terminate(); }
 
-Status HostProcess::GetMainModule(FileSpec &file_spec) const {
-  return m_native_process->GetMainModule(file_spec);
-}
-
 lldb::pid_t HostProcess::GetProcessId() const {
   return m_native_process->GetProcessId();
 }

diff  --git a/lldb/source/Host/posix/HostProcessPosix.cpp 
b/lldb/source/Host/posix/HostProcessPosix.cpp
index 8599a94d22416..9889be07bca8b 100644
--- a/lldb/source/Host/posix/HostProcessPosix.cpp
+++ b/lldb/source/Host/posix/HostProcessPosix.cpp
@@ -49,31 +49,6 @@ Status HostProcessPosix::Signal(lldb::process_t process, int 
signo) {
 
 Status HostProcessPosix::Terminate() { return Signal(SIGKILL); }
 
-Status HostProcessPosix::GetMainModule(FileSpec &file_spec) const {
-  Status error;
-
-  // Use special code here because proc/[pid]/exe is a symbolic link.
-  char link_path[PATH_MAX];
-  if (snprintf(link_path, PATH_MAX, "/proc/%" PRIu64 "/exe", m_process) != 1) {
-error.SetErrorString("Unable to build /proc//exe string");
-return error;
-  }
-
-  error = FileSystem::Instance().Readlink(FileSpec(link_path), file_spec);
-  if (!error.Success())
-return error;
-
-  // If the binary has been deleted, the link name has " (deleted)" appended.
-  // Remove if there.
-  if (file_spec.GetFilename().GetStringRef().endswith(" (deleted)")) {
-const char *filename = file_spec.GetFilename().GetCString();
-static const size_t deleted_len = strlen(" (deleted)");
-const size_t len = file_spec.GetFilename().GetLength();
-file_spec.GetFilename().SetCStringWithLength(filename, len - deleted_len);
-  }
-  return error;
-}
-
 lldb::pid_t HostProcessPosix::GetProces

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

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

@jasonmolenda one advantage this has over grepping is that the python specific 
extensions to SB are visible.


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] D120292: [lldb] Add lldb.find helper function

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/bindings/python/python-extensions.swig:528-533
+def find(pattern, flags=re.I):
+"""
+Find matching SB methods/properties. See also Python's builtin `help()`.
+"""
+import lldb
+import re

oh, I need to figure out the right way to manage imports in this swig 
extension. It seems that the `re` module is used (as a default argument) before 
importing it, and yet this code works.


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] D120292: [lldb] Add lldb.find helper function

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/bindings/python/python-extensions.swig:528-533
+def find(pattern, flags=re.I):
+"""
+Find matching SB methods/properties. See also Python's builtin `help()`.
+"""
+import lldb
+import re

kastiglione wrote:
> oh, I need to figure out the right way to manage imports in this swig 
> extension. It seems that the `re` module is used (as a default argument) 
> before importing it, and yet this code works.
Can't you just import it after the `%pythoncode %{` statement instead of doing 
it in the function ?


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] D120292: [lldb] Add lldb.find helper function

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/bindings/python/python-extensions.swig:528-533
+def find(pattern, flags=re.I):
+"""
+Find matching SB methods/properties. See also Python's builtin `help()`.
+"""
+import lldb
+import re

mib wrote:
> kastiglione wrote:
> > oh, I need to figure out the right way to manage imports in this swig 
> > extension. It seems that the `re` module is used (as a default argument) 
> > before importing it, and yet this code works.
> Can't you just import it after the `%pythoncode %{` statement instead of 
> doing it in the function ?
That's what I plan to try. I figure it's best to put a `%pythoncode %{ ` at the 
top of the file, where all imports can go, like a normal source file.


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] D120292: [lldb] Add lldb.find helper function

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/bindings/python/python-extensions.swig:528-533
+def find(pattern, flags=re.I):
+"""
+Find matching SB methods/properties. See also Python's builtin `help()`.
+"""
+import lldb
+import re

kastiglione wrote:
> mib wrote:
> > kastiglione wrote:
> > > oh, I need to figure out the right way to manage imports in this swig 
> > > extension. It seems that the `re` module is used (as a default argument) 
> > > before importing it, and yet this code works.
> > Can't you just import it after the `%pythoncode %{` statement instead of 
> > doing it in the function ?
> That's what I plan to try. I figure it's best to put a `%pythoncode %{ ` at 
> the top of the file, where all imports can go, like a normal source file.
👍🏼


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] D120292: [lldb] Add lldb.find helper function

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D120292#3338155 , @kastiglione 
wrote:

> @jasonmolenda one advantage this has over grepping is that the python 
> specific extensions to SB are visible.

That's true too.  I mostly meant my comment as in, "I find this feature very 
helpful when writing SB API python, and this provides it to people who don't 
have an lldb checkout sitting around".


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] D120322: [lldb] Simplify HostThreadMacOSX

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm:17-20
+  NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
+  lldb::thread_result_t result = HostThreadPosix::ThreadCreateTrampoline(arg);
+  [pool drain];
+  return result;

I believe it's recommended to use an @autoreleasepool block instead of using 
the NSAutoreleasePool directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120322

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

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Nice cleanup




Comment at: lldb/source/Host/common/HostNativeThreadBase.cpp:55-56
 HostNativeThreadBase::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ThreadLauncher::HostThreadCreateInfo *info =
-  (ThreadLauncher::HostThreadCreateInfo *)arg;
-  llvm::set_thread_name(info->thread_name);
-
-  thread_func_t thread_fptr = info->thread_fptr;
-  thread_arg_t thread_arg = info->thread_arg;
+  std::unique_ptr info_up(
+  (ThreadLauncher::HostThreadCreateInfo *)arg);
+  llvm::set_thread_name(info_up->thread_name);

make_unique maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120321

___
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 410679.
zequanwu added a comment.

- Use `-show-variable-ranges/-R single/all` to dump single addres range that 
contains the querying address or all address ranges. It seeme like I can't use 
non-printable character like `\\x01` to omit short option when it takes a 
parameter, so `-R` is used as abbrev "range".
- Refactor `DWARFExpression::GetLocationExpression`.


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/lldb-private-enumerations.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Address.cpp
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s
@@ -8,10 +8,32 @@
 # RUN: %lldb %t -o "image lookup -v -a 0" -o "image lookup -v -a 2" \
 # RUN:   -o "image dump symfile" -o exit | FileCheck %s
 
+# RUN: %lldb %t -o "image lookup -v -a 0 -R s" -o "image lookup -v -a 2 -R s" \
+# RUN: -o exit | FileCheck %s --check-prefix=SINGLE-RANGE
+
+# RUN: %lldb %t -o "image lookup -v -a 0 -R a" -o "image lookup -v -a 2 -R a" \
+# RUN: -o exit | FileCheck %s --check-prefix=ALL-RANGES
+
 # RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s --defsym LOCLISTS=0 > %t
 # RUN: %lldb %t -o "image lookup -v -a 0" -o "image lookup -v -a 2" \
 # RUN:   -o "image dump symfile" -o exit | FileCheck %s --check-prefix=CHECK --check-prefix=LOCLISTS
 
+# SINGLE-RANGE-LABEL: image lookup -v -a 0 -R s
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x0", type = "int", valid ranges =, location = [0x, 0x0001) -> DW_OP_reg5 RDI, decl =
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x1", type = "int", valid ranges =, location = , decl =
+# SINGLE-RANGE-LABEL: image lookup -v -a 2 -R s
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x0", type = "int", valid ranges =, location = [0x0001, 0x0006) -> DW_OP_reg0 RAX, decl =
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x1", type = "int", valid ranges =, location = , decl =
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x3", type = "int", valid ranges =, location = [0x0002, 0x0003) -> DW_OP_reg1 RDX, decl =
+
+# ALL-RANGES-LABEL: image lookup -v -a 0 -R a
+# ALL-RANGES: Variable: id = {{.*}}, name = "x0", type = "int", valid ranges =, location = [0x, 0x0001) -> DW_OP_reg5 RDI, [0x0001, 0x0006) -> DW_OP_reg0 RAX, decl =
+# ALL-RANGES: Variable: id = {{.*}}, name = "x1", type = "int", valid ranges =, location = , decl =
+# ALL-RANGES-LABEL: image lookup -v -a 2 -R a
+# ALL-RANGES:  Variable: id = {{.*}}, name = "x0", type = "int", valid ranges =, location = [0x, 0x0001) -> DW_OP_reg5 RDI, [0x0001, 0x0006) -> DW_OP_reg0 RAX, decl =
+# ALL-RANGES: Variable: id = {{.*}}, name = "x1", type = "int", valid ranges =, location = , decl =
+# ALL-RANGES: Variable: id = {{.*}}, name = "x3", type = "int", valid ranges =, location = [0x0002, 0x0003) -> DW_OP_reg1 RDX, decl =
+
 # CHECK-LABEL: image lookup -v -a 0
 # CHECK: Variable: {{.*}}, name = "x0", type = "int", location = DW_OP_reg5 RDI,
 # CHECK: Variable: {{.*}}, name = "x1", type = "int", location = ,
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2673,14 +2673,40 @@
   return DataExtractor(buffer_sp, byte_order, addr_size);
 }
 
-llvm::Optional
-DWARFExpression::GetLocationExpression(addr_t load_function_start,
-   addr_t addr) const {
+bool DWARFExpression::DumpLocations(
+Stream *s, lldb::DescriptionLevel level, lldb::addr_t load_function_start,
+ABI *abi,
+llvm::function_ref filter,
+llvm::function_ref range_printer) {
+  if (!IsLocationList()) {
+DumpLocation(s, m_data, level, abi);
+return true;
+  }
+  bool is_first = true;
+  auto callback = [&](llvm::DWARFLocationExpression loc) -> bool {
+bool should_continue = true;
+if (loc.Range && filter(*loc.Range, m_loclist_addresses->func_file_addr,
+should_continue)) {
+  DataExtractor data = ToDataExtractor(loc, m_data.GetByteOrder(),
+   m_data.GetAddressByteSize());
+  range_printer(*loc.Range, is_first);
+  DumpLocation(s, data, level, abi);
+  is_first = false;
+}
+return should_continue;
+  };
+  if (!GetLocati

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

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



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

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 
> > > defining the interactions of the two fields (if you still want to pursue 
> > > this, that is).
> > > 
> > > So how about this: if the user passes the extra flag, then we print both 
> > > the range field (if it exists), and the *full* location list (in that 
> > > order, ideally). That way the output will be either `range = [a, b), [c, 
> > > d), location = DW_OP_reg47` or `location = [a,b) -> DW_OP_reg4, [c,d) -> 
> > > DW_OP_reg7`. If the dwarf plugin starts using the range field again then 
> > > the output will contain both fields, which will be slightly confusing, 
> > > but at least not misleading (and we can also change the format then).
> > Oh, I think I misunderstood `m_scope_range`. It's the range list where the 
> > variable is valid regardless whether its value is accessible or not (valid 
> > range). As for `m_location` in `Variable`, it's describing the ranges where 
> > the value is (value range). They are not the same. 
> > 
> > So, currently how NativePDB creates local Variable's range is not correct. 
> > That only works when it's not optimized build such that the valid range is 
> > the same as the value range. It still need to create dwarf location lists 
> > to correctly represent the value range, but as mentioned [[ 
> > https://reviews.llvm.org/D119508#3319113 | here ]], we need to choose a 
> > generic "variable location provider" interface for that.
> > 
> > Oh, I think I misunderstood m_scope_range. It's the range list where the 
> > variable is valid regardless whether its value is accessible or not (valid 
> > range). As for m_locatio