[lldb] Implement basic support for reverse-continue #99736
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: Robert O'Callahan (rocallahan) ChangesThis commit only adds support for the This feature depends on a gdbserver implementation (e.g. The majority of this PR is test infrastructure (about 700 of the 950 lines added). Patch is 56.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/99736.diff 25 Files Affected:
diff --git a/lldb/include/lldb/API/SBProcess.h b/lldb/include/lldb/API/SBProcess.h
index 778be79583990..9b17bac0093e6 100644
--- a/lldb/include/lldb/API/SBProcess.h
+++ b/lldb/include/lldb/API/SBProcess.h
@@ -160,6 +160,8 @@ class LLDB_API SBProcess {
lldb::SBError Continue();
+ lldb::SBError ReverseContinue();
+
lldb::SBError Stop();
lldb::SBError Kill();
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index c8475db8ae160..203d3484f3704 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -874,10 +874,10 @@ class Process : public std::enable_shared_from_this<Process>,
/// \see Thread:Resume()
/// \see Thread:Step()
/// \see Thread:Suspend()
- Status Resume();
+ Status Resume(lldb::RunDirection direction = lldb::eRunForward);
/// Resume a process, and wait for it to stop.
- Status ResumeSynchronous(Stream *stream);
+ Status ResumeSynchronous(Stream *stream, lldb::RunDirection direction = lldb::eRunForward);
/// Halts a running process.
///
@@ -1136,6 +1136,22 @@ class Process : public std::enable_shared_from_this<Process>,
return error;
}
+ /// Like DoResume() but executes in reverse if supported.
+ ///
+ /// \return
+ /// Returns \b true if the process successfully resumes using
+ /// the thread run control actions, \b false otherwise.
+ ///
+ /// \see Thread:Resume()
+ /// \see Thread:Step()
+ /// \see Thread:Suspend()
+ virtual Status DoResumeReverse() {
+ Status error;
+ error.SetErrorStringWithFormatv(
+ "error: {0} does not support reverse execution of processes", GetPluginName());
+ return error;
+ }
+
/// Called after resuming a process.
///
/// Allow Process plug-ins to execute some code after resuming a process.
@@ -2367,6 +2383,8 @@ class Process : public std::enable_shared_from_this<Process>,
bool IsRunning() const;
+ lldb::RunDirection GetLastRunDirection() { return m_last_run_direction; }
+
DynamicCheckerFunctions *GetDynamicCheckers() {
return m_dynamic_checkers_up.get();
}
@@ -2861,7 +2879,7 @@ void PruneThreadPlans();
///
/// \return
/// An Status object describing the success or failure of the resume.
- Status PrivateResume();
+ Status PrivateResume(lldb::RunDirection direction = lldb::eRunForward);
// Called internally
void CompleteAttach();
@@ -3139,6 +3157,7 @@ void PruneThreadPlans();
// m_currently_handling_do_on_removals are true,
// Resume will only request a resume, using this
// flag to check.
+ lldb::RunDirection m_last_run_direction;
/// This is set at the beginning of Process::Finalize() to stop functions
/// from looking up or creating things during or after a finalize call.
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index d1848fcbbbdb1..f49854275653e 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -138,6 +138,9 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
static lldb::StopInfoSP
CreateStopReasonProcessorTrace(Thread &thread, const char *description);
+ static lldb::StopInfoSP
+ CreateStopReasonHistoryBoundary(Thread &thread, const char *description);
+
static lldb::StopInfoSP CreateStopReasonFork(Thread &thread,
lldb::pid_t child_pid,
lldb::tid_t child_tid);
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 74ff458bf87de..509f1b76934d2 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -135,6 +135,9 @@ FLAGS_ENUM(LaunchFlags){
/// Thread Run Modes.
enum RunMode { eOnlyThisThread, eAllThreads, eOnlyDuringStepping };
+/// Execution directions
+enum RunDirection { eRunForward, eRunReverse };
+
/// Byte ordering definitions.
enum ByteOrder {
eByteOrderInvalid = 0,
@@ -253,6 +256,7 @@ enum StopReason {
eStopReasonFork,
eStopReasonVFork,
eStopReasonVForkDone,
+ eStopReasonHistoryBoundary,
};
/// Command Return Status Types.
diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
index 1784487323ad6..732d617132068 100644
--- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
+++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
@@ -510,8 +510,9 @@ def start(self):
self._thread.start()
def stop(self):
- self._thread.join()
- self._thread = None
+ if self._thread is not None:
+ self._thread.join()
+ self._thread = None
def get_connect_address(self):
return self._socket.get_connect_address()
diff --git a/lldb/packages/Python/lldbsuite/test/lldbgdbproxy.py b/lldb/packages/Python/lldbsuite/test/lldbgdbproxy.py
new file mode 100644
index 0000000000000..45018e7daa7df
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/lldbgdbproxy.py
@@ -0,0 +1,176 @@
+import logging
+import os
+import os.path
+import random
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.gdbclientutils import *
+import lldbgdbserverutils
+from lldbsuite.support import seven
+
+
+class GDBProxyTestBase(TestBase):
+ """
+ Base class for gdbserver proxy tests.
+
+ This class will setup and start a mock GDB server for the test to use.
+ It pases through requests to a regular lldb-server/debugserver and
+ forwards replies back to the LLDB under test.
+ """
+
+ """The gdbserver that we implement."""
+ server = None
+ """The inner lldb-server/debugserver process that we proxy requests into."""
+ monitor_server = None
+ monitor_sock = None
+
+ server_socket_class = TCPServerSocket
+
+ DEFAULT_TIMEOUT = 20 * (10 if ("ASAN_OPTIONS" in os.environ) else 1)
+
+ _verbose_log_handler = None
+ _log_formatter = logging.Formatter(fmt="%(asctime)-15s %(levelname)-8s %(message)s")
+
+ def setUpBaseLogging(self):
+ self.logger = logging.getLogger(__name__)
+
+ if len(self.logger.handlers) > 0:
+ return # We have set up this handler already
+
+ self.logger.propagate = False
+ self.logger.setLevel(logging.DEBUG)
+
+ # log all warnings to stderr
+ handler = logging.StreamHandler()
+ handler.setLevel(logging.WARNING)
+ handler.setFormatter(self._log_formatter)
+ self.logger.addHandler(handler)
+
+ def setUp(self):
+ TestBase.setUp(self)
+
+ self.setUpBaseLogging()
+
+ if self.isVerboseLoggingRequested():
+ # If requested, full logs go to a log file
+ log_file_name = self.getLogBasenameForCurrentTest() + "-proxy.log"
+ self._verbose_log_handler = logging.FileHandler(
+ log_file_name
+ )
+ self._verbose_log_handler.setFormatter(self._log_formatter)
+ self._verbose_log_handler.setLevel(logging.DEBUG)
+ self.logger.addHandler(self._verbose_log_handler)
+
+ self.port = self.get_next_port()
+ lldb_server_exe = lldbgdbserverutils.get_lldb_server_exe()
+ if lldb_server_exe is None:
+ self.debug_monitor_exe = lldbgdbserverutils.get_debugserver_exe()
+ self.assertTrue(self.debug_monitor_exe is not None)
+ self.debug_monitor_extra_args = []
+ else:
+ self.debug_monitor_exe = lldb_server_exe
+ self.debug_monitor_extra_args = ["gdbserver"]
+
+ self.server = MockGDBServer(self.server_socket_class())
+ self.server.responder = self
+
+ def tearDown(self):
+ # TestBase.tearDown will kill the process, but we need to kill it early
+ # so its client connection closes and we can stop the server before
+ # finally calling the base tearDown.
+ if self.process() is not None:
+ self.process().Kill()
+ self.server.stop()
+
+ self.logger.removeHandler(self._verbose_log_handler)
+ self._verbose_log_handler = None
+
+ TestBase.tearDown(self)
+
+ def isVerboseLoggingRequested(self):
+ # We will report our detailed logs if the user requested that the "gdb-remote" channel is
+ # logged.
+ return any(("gdb-remote" in channel) for channel in lldbtest_config.channels)
+
+ def connect(self, target):
+ """
+ Create a process by connecting to the mock GDB server.
+ """
+ self.prep_debug_monitor_and_inferior()
+ self.server.start()
+
+ listener = self.dbg.GetListener()
+ error = lldb.SBError()
+ process = target.ConnectRemote(
+ listener, self.server.get_connect_url(), "gdb-remote", error
+ )
+ self.assertTrue(error.Success(), error.description)
+ self.assertTrue(process, PROCESS_IS_VALID)
+ return process
+
+ def get_next_port(self):
+ return 12000 + random.randint(0, 3999)
+
+ def prep_debug_monitor_and_inferior(self):
+ inferior_exe_path = self.getBuildArtifact("a.out")
+ self.connect_to_debug_monitor([inferior_exe_path])
+ self.assertIsNotNone(self.monitor_server)
+ self.initial_handshake()
+
+ def initial_handshake(self):
+ self.monitor_server.send_packet(seven.bitcast_to_bytes("+"))
+ reply = seven.bitcast_to_string(self.monitor_server.get_normal_packet())
+ self.assertEqual(reply, "+")
+ self.monitor_server.send_packet(seven.bitcast_to_bytes("QStartNoAckMode"))
+ reply = seven.bitcast_to_string(self.monitor_server.get_normal_packet())
+ self.assertEqual(reply, "+")
+ reply = seven.bitcast_to_string(self.monitor_server.get_normal_packet())
+ self.assertEqual(reply, "OK")
+ self.monitor_server.send_packet(seven.bitcast_to_bytes("+"))
+ reply = seven.bitcast_to_string(self.monitor_server.get_normal_packet())
+ self.assertEqual(reply, "+")
+
+ def get_debug_monitor_command_line_args(self, connect_address, launch_args):
+ return self.debug_monitor_extra_args + ["--reverse-connect", connect_address] + launch_args
+
+ def launch_debug_monitor(self, launch_args):
+ family, type, proto, _, addr = socket.getaddrinfo(
+ "localhost", 0, proto=socket.IPPROTO_TCP
+ )[0]
+ sock = socket.socket(family, type, proto)
+ sock.settimeout(self.DEFAULT_TIMEOUT)
+ sock.bind(addr)
+ sock.listen(1)
+ addr = sock.getsockname()
+ connect_address = "[{}]:{}".format(*addr)
+
+ commandline_args = self.get_debug_monitor_command_line_args(
+ connect_address, launch_args
+ )
+
+ # Start the server.
+ self.logger.info(f"Spawning monitor {commandline_args}")
+ monitor_process = self.spawnSubprocess(
+ self.debug_monitor_exe, commandline_args, install_remote=False
+ )
+ self.assertIsNotNone(monitor_process)
+
+ self.monitor_sock = sock.accept()[0]
+ self.monitor_sock.settimeout(self.DEFAULT_TIMEOUT)
+ return monitor_process
+
+ def connect_to_debug_monitor(self, launch_args):
+ monitor_process = self.launch_debug_monitor(launch_args)
+ self.monitor_server = lldbgdbserverutils.Server(self.monitor_sock, monitor_process)
+
+ def respond(self, packet):
+ """Subclasses can override this to change how packets are handled."""
+ return self.pass_through(packet)
+
+ def pass_through(self, packet):
+ self.logger.info(f"Sending packet {packet}")
+ self.monitor_server.send_packet(seven.bitcast_to_bytes(packet))
+ reply = seven.bitcast_to_string(self.monitor_server.get_normal_packet())
+ self.logger.info(f"Received reply {reply}")
+ return reply
diff --git a/lldb/packages/Python/lldbsuite/test/lldbreverse.py b/lldb/packages/Python/lldbsuite/test/lldbreverse.py
new file mode 100644
index 0000000000000..a1fb396bd921f
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/lldbreverse.py
@@ -0,0 +1,415 @@
+import os
+import os.path
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbproxy import *
+import lldbgdbserverutils
+import re
+
+
+class ThreadSnapshot:
+ def __init__(self, thread_id, registers):
+ self.thread_id = thread_id
+ self.registers = registers
+
+
+class MemoryBlockSnapshot:
+ def __init__(self, address, data):
+ self.address = address
+ self.data = data
+
+
+class StateSnapshot:
+ def __init__(self, thread_snapshots, memory):
+ self.thread_snapshots = thread_snapshots
+ self.memory = memory
+ self.thread_id = None
+
+
+class RegisterInfo:
+ def __init__(self, lldb_index, bitsize, little_endian):
+ self.lldb_index = lldb_index
+ self.bitsize = bitsize
+ self.little_endian = little_endian
+
+
+BELOW_STACK_POINTER = 16384
+ABOVE_STACK_POINTER = 4096
+
+BLOCK_SIZE = 1024
+
+SOFTWARE_BREAKPOINTS = 0
+HARDWARE_BREAKPOINTS = 1
+WRITE_WATCHPOINTS = 2
+
+
+class ReverseTestBase(GDBProxyTestBase):
+ """
+ Base class for tests that need reverse execution.
+
+ This class uses a gdbserver proxy to add very limited reverse-
+ execution capability to lldb-server/debugserver for testing
+ purposes only.
+
+ To use this class, run the inferior forward until some stopping point.
+ Then call `start_recording()` and execute forward again until reaching
+ a software breakpoint; this class records the state before each execution executes.
+ At that point, the server will accept "bc" and "bs" packets to step
+ backwards through the state.
+ When executing during recording, we only allow single-step and continue without
+ delivering a signal, and only software breakpoint stops are allowed.
+
+ We assume that while recording is enabled, the only effects of instructions
+ are on general-purpose registers (read/written by the 'g' and 'G' packets)
+ and on memory bytes between [SP - BELOW_STACK_POINTER, SP + ABOVE_STACK_POINTER).
+ """
+
+ """
+ A list of StateSnapshots in time order.
+
+ There is one snapshot per single-stepped instruction,
+ representing the state before that instruction was
+ executed. The last snapshot in the list is the
+ snapshot before the last instruction was executed.
+ This is an undo log; we snapshot a superset of the state that may have
+ been changed by the instruction's execution.
+ """
+ snapshots = None
+ recording_enabled = False
+
+ breakpoints = None
+
+ pid = None
+
+ pc_register_info = None
+ sp_register_info = None
+ general_purpose_register_info = None
+
+ def __init__(self, *args, **kwargs):
+ GDBProxyTestBase.__init__(self, *args, **kwargs)
+ self.breakpoints = [set(), set(), set(), set(), set()]
+
+ def respond(self, packet):
+ if not packet:
+ raise ValueError("Invalid empty packet")
+ if self.is_command(packet, "qSupported", ":"):
+ reply = self.pass_through(packet)
+ return reply + ";ReverseStep+;ReverseContinue+"
+ if self.is_command(packet, "vCont", ";"):
+ if self.recording_enabled:
+ return self.continue_with_recording(packet)
+ snapshots = []
+ if packet[0] == "c" or packet[0] == "s" or packet[0] == "C" or packet[0] == "S":
+ raise ValueError("LLDB should not be sending old-style continuation packets")
+ if packet == "bc":
+ return self.reverse_continue()
+ if packet == "bs":
+ return self.reverse_step()
+ if packet == 'jThreadsInfo':
+ # Suppress this because it contains thread stop reasons which we might
+ # need to modify, and we don't want to have to implement that.
+ return ""
+ if packet[0] == "z" or packet[0] == "Z":
+ reply = self.pass_through(packet)
+ if reply == "OK":
+ self.update_breakpoints(packet)
+ return reply
+ return GDBProxyTestBase.respond(self, packet)
+
+ def start_recording(self):
+ self.recording_enabled = True
+ self.snapshots = []
+
+ def stop_recording(self):
+ """
+ Don't record when executing foward.
+
+ Reverse execution is still supported until the next forward continue.
+ """
+ self.recording_enabled = False
+
+ def is_command(self, packet, cmd, follow_token):
+ return packet == cmd or packet[0:len(cmd) + 1] == cmd + follow_token
+
+ def update_breakpoints(self, packet):
+ m = re.match("([zZ])([01234]),([0-9a-f]+),([0-9a-f]+)", packet)
+ if m is None:
+ raise ValueError("Invalid breakpoint packet: " + packet)
+ t = int(m.group(2))
+ addr = int(m.group(3), 16)
+ kind = int(m.group(4), 16)
+ if m.group(1) == 'Z':
+ self.breakpoints[t].add((addr, kind))
+ else:
+ self.breakpoints[t].discard((addr, kind))
+
+ def breakpoint_triggered_at(self, pc):
+ if any(addr == pc for addr, kind in self.breakpoints[SOFTWARE_BREAKPOINTS]):
+ return True
+ if any(addr == pc for addr, kind in self.breakpoints[HARDWARE_BREAKPOINTS]):
+ return True
+ return False
+
+ def watchpoint_triggered(self, new_value_block, current_contents):
+ """Returns the address or None."""
+ for watch_addr, kind in breakpoints[WRITE_WATCHPOINTS]:
+ for offset in range(0, kind):
+ addr = watch_addr + offset
+ if (addr >= new_value_block.address and
+ addr < new_value_block.address + len(new_value_block.data)):
+ index = addr - new_value_block.address
+ if new_value_block.data[index*2:(index + 1)*2] != current_contents[index*2:(index + 1)*2]:
+ return watch_addr
+ return None
+
+ def continue_with_recording(self, packet):
+ self.logger.debug("Continue with recording enabled")
+
+ step_packet = "vCont;s"
+ if packet == "vCont":
+ requested_step = False
+ else:
+ m = re.match("vCont;(c|s)(.*)", packet)
+ if m is None:
+ raise ValueError("Unsupported vCont packet: " + packet)
+ requested_step = m.group(1) == 's'
+ step_packet += m.group(2)
+
+ while True:
+ snapshot = self.capture_snapshot()
+ reply = self.pass_through(step_packet)
+ (stop_signal, stop_pairs) = self.parse_stop(reply)
+ if stop_signal != 5:
+ raise ValueError("Unexpected stop signal: " + reply)
+ is_swbreak = False
+ thread_id = None
+ for key, value in stop_pairs.items():
+ if key == "thread":
+ thread_id = self.parse_thread_id(value)
+ continue
+ if re.match('[0-9a-f]+', key):
+ continue
+ if key == "swbreak" or (key == "reason" and value == "breakpoint"):
+ is_swbreak = True
+ continue
+ if key in ["name", "threads", "thread-pcs", "reason"]:
+ continue
+ raise ValueError(f"Unknown stop key '{key}' in {reply}")
+ if is_swbreak:
+ self.logger.debug("Recording stopped")
+ return reply
+ if thread_id is None:
+ return ValueError("Expected thread ID: " + reply)
+ snapshot.thread_id = thread_id
+ self.snapshots.append(snapshot)
+ if requested_step:
+ self.logger.debug("Recording stopped for step")
+ return reply
+
+ def parse_stop(self, reply):
+ result = {}
+ if not reply:
+ raise ValueError("Invalid empty packet")
+ if reply[0] == "T" and len(reply) >= 3:
+ result = {k:v for k, v in self.parse_pairs(reply[3:])}
+ return (int(reply[1:3], 16), result)
+ raise "Unsupported stop reply: " + reply
+
+ def parse_pairs(self, text):
+ for pair in text.split(";"):
+ if not pair:
+ continu...
[truncated]
|
|
BTW this was discussed in https://discourse.llvm.org/t/how-to-test-reverse-execution-support-in-lldb/79696. |
There was a problem hiding this comment.
This is very interesting, I've left a few comments here and there but I've noticed you didn't check if this works both when the debugger is in synchronous and asynchronous in your tests. This is definitely something you want to exercise since it changes lldb's execution control behavior.
Also, I'm not sure what additional information does the History Boundary stop reason provides. Can it actually provide historical data to the user ? If so, I'm not sure the stop reason is the best place to put it. May be it would be worth adding a new subcommand to the trace top-level command ?
Thanks!
lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py
Show resolved
Hide resolved
lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py
Show resolved
Hide resolved
|
This is very exciting! |
OK, I've added this.
When a user reverse-continues to the start of recorded time (typically the point where the target was initially execed), execution stops and we must report a reason. None of the existing reasons are appropriate so I created this new one. I've added an explanatory comment to its definition in |
rocallahan
force-pushed
the
reverse-continue
branch
from
ce13a71 to
25e184d
Compare
jimingham
reviewed
Jul 22, 2024
jimingham
reviewed
Jul 22, 2024
jimingham
reviewed
Jul 22, 2024
jimingham
reviewed
Jul 22, 2024
|
On Jul 22, 2024, at 2:52 PM, rocallahan ***@***.***> wrote:
@rocallahan commented on this pull request.
In lldb/source/Target/Process.cpp <#99736 (comment)>:
> @@ -3264,6 +3266,11 @@ Status Process::PrivateResume() {
// filters before resuming.
UpdateAutomaticSignalFiltering();
+ if (m_last_run_direction != direction) {
TBH I don't understand thread plans very well yet so I was trying to avoid interacting with them. I guess implementing your suggestion means extending thread plans with a RunDirection, and/or creating new plan subclasses? And then we'd have to test a lot of interactions. Would it be OK to defer that to a future patch to keep this patch reasonably small?
Oh, for sure.
In the end you will have to do something of that sort. Otherwise you'll get unexpected behavior mixing forward & backwards stepping. Plans that complete without interruption should be fine, but ones returning control to the user will start to cancel each other out, and that will get confusing.
However, straight-line execution forward or backwards should be fine, and forward/backwards continue are not affected by this either. So this should still be mostly functional without the added complexity, making it appropriate to do that work as a follow-on.
Jim
… —
Reply to this email directly, view it on GitHub <#99736 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7XSBUPBPIAKW2RCU3ZNV5IJAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJSGYYTCMRVGI>.
You are receiving this because you are on a team that was mentioned.
|
|
On Jul 22, 2024, at 2:56 PM, rocallahan ***@***.***> wrote:
@rocallahan commented on this pull request.
In lldb/source/Target/Process.cpp <#99736 (comment)>:
> @@ -3264,6 +3266,11 @@ Status Process::PrivateResume() {
// filters before resuming.
UpdateAutomaticSignalFiltering();
+ if (m_last_run_direction != direction) {
If the user does "continue", hits a breakpoint, does "reverse-step", and then "reverse-continue" presumably we do have to abandon the original "continue". Since this patch only implements "reverse-continue" maybe always abandoning the thread plans is actually the right thing to do for now?
"Continue" isn't a persistent operation for thread plans. The thing that would be "persistent" in this case would be the "ThreadPlanStepOverRange(forward)" plan which the "next" operation pushed on the stack. When you hit the breakpoint, that plan was asked "did you explain this stop" - the answer to that was "no" this was just a random breakpoint hit. It was asked also "are you done" - the answer to which was still no since the next-ing frame is still on the stack, and finally "are you stale - no longer relevant" but the frame is still on the Thread's stack, so that was answered "false" the "ThreadPlanStepOverRange stayed on the ThreadPlan stack.
Then in my scenario I did a couple of forward steps, and some "reverse steps". At each point the next plan was asked the same question, and so long as that StackFrame is still on the thread stack, the plan stays active.
If you did a reverse-continue that stopped when the process is in a state where the next-ing StackFrame is no longer on the stack, that plan will return "false" to the "is stale" question and get popped at that point.
But so long as the reverse continues stays in the time-line where that frame is on the stack, we should honor the user intentions that came with it.
There are some things you will have to do to make all this really work (for instance, a "forward-next" plan can't explain stops from backward execution, so the plans will have to know about that.
As I said on another thread, I think you will have to make the plans cooperate with forward and backward behavior at some point. But for this patch it's fine to not allow that mixing, and in that case it is probably best to just dump all the extant and dumpable plans.
You might have to be careful in the case where someone runs a function call, stops at a breakpoint in the function call and decides to go backwards in the function execution (if that's possible). Dumping the function plan removes the function calling frames from the stack, so if you throw away that plan you will end up exiting the function call - which wasn't what the user expected.
Jim
… —
Reply to this email directly, view it on GitHub <#99736 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7PMAASCC4KYT6KBBDZNV5ZHAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJSGYYTKOJTGU>.
You are receiving this because you are on a team that was mentioned.
|
That is not possible in rr. During user function calls it responds to reverse execution requests with an immediate stop. |
|
On Jul 22, 2024, at 2:54 PM, rocallahan ***@***.***> wrote:
@rocallahan commented on this pull request.
In lldb/source/Target/Process.cpp <#99736 (comment)>:
> @@ -3264,6 +3266,11 @@ Status Process::PrivateResume() {
// filters before resuming.
UpdateAutomaticSignalFiltering();
+ if (m_last_run_direction != direction) {
I'm a bit worried that in general thread plans have a lot of complexity, and adding more complexity by allowing multiple stacked plans with different directions might be bad enough for maintainability that it's overall a bad idea.
I hope that's not true. I don't think we need to have the thread plans have some pinning to the time-line of the user's actions. For instance, if you were doing a "forward-next", hit a breakpoint and did some forward and backwards stepping, then did a backwards continue to a point where that thread no longer existed, then a forward continue back to where you started from, we do NOT need to preserve the next behavior at that point. However, so long as the forward and backward operations always happen under the frame we were next-ing on, we should be able to preserve the behavior in that case.
It shouldn't be too unnatural, every time we stop we do a sanity check on the plans to see if they are still relevant before we return control to the user. For instance, if you were nexting over a call that threw an exception, which was caught above the frame you were `nexting` in we have to do the same sort of "is stale" pruning. As long as we're appropriately firm about pruning plans as they stop being relevant, this should be manageable.
Jim
… —
Reply to this email directly, view it on GitHub <#99736 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3VEF6SXZPEBS5W5KDZNV5RDAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJSGYYTGNRYGA>.
You are receiving this because you are on a team that was mentioned.
|
|
On Jul 22, 2024, at 3:44 PM, rocallahan ***@***.***> wrote:
You might have to be careful in the case where someone runs a function call, stops at a breakpoint in the function call and decides to go backwards in the function execution (if that's possible).
That is not possible in rr. During user function calls it responds to reverse execution requests with an immediate stop.
So long as you haven't pruned the plans that control the function call before doing that, you should be fine.
Jim
… —
Reply to this email directly, view it on GitHub <#99736 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7T7QNKD3CRL3QOOSLZNWDLPAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBTHEZTKNZVGY>.
You are receiving this because you are on a team that was mentioned.
|
rocallahan
force-pushed
the
reverse-continue
branch
2 times, most recently
from
5ecca71 to
8d23087
Compare
labath
reviewed
Jul 23, 2024
lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py
Outdated
Show resolved
Hide resolved
clayborg
requested changes
Jul 23, 2024
Comment on lines
-877
to
+860
| Status Resume(); | ||
| Status Resume(lldb::RunDirection direction = lldb::eRunForward); |
There was a problem hiding this comment.
internal APIs, anything not in the lldb namespace can be changed. So this is ok. Though I personally would like to see a:
Status ReverseResume();
I am open to feedback from other here as my mind can easily be changed.
There was a problem hiding this comment.
The decision was to use a direction flag.
jimingham
reviewed
Jul 23, 2024
|
On Jul 23, 2024, at 9:47 AM, Greg Clayton ***@***.***> wrote:
@clayborg requested changes on this pull request.
In lldb/include/lldb/API/SBProcess.h <#99736 (comment)>:
> - lldb::SBError Continue();
+ lldb::SBError Continue(RunDirection direction = RunDirection::eRunForward);
Our public API has rules:
can't change any existing API call, you can add an overload, but you can't change any public APIs that are already there. Anything in the lldb namespace can't be changed.
no virtual functions
can't change any ivars or the size of the object
That being said, I would rather have a:
lldb::SBError ReverseContinue();
call than have everyone using the Continue API to say wether they want to go forward or in reverse.
In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
> - Status Resume();
+ Status Resume(lldb::RunDirection direction = lldb::eRunForward);
internal APIs, anything not in the lldb namespace can be changed. So this is ok. Though I personally would like to see a:
Status ReverseResume();
I am open to feedback from other here as my mind can easily be changed.
You just have to omit the default argument, and leave the 0 argument form in place. That will have the effect you want, all previous code will use the forward continue, and new code that wants to be explicit can pass the argument.
I asked for this change, because it seems like where you are going to use it you will have some variable telling yourself which direction you are going, and so if there's an argument, you will find yourself writing:
my_process.Continue(the_direction_parameter_i_already_had)
as opposed to
if the_direction_parameter_i_already_had == lldb::eRunForward:
my_process.Continue()
else:
my_process.ReverseContinue()
The former seemed much nicer to me.
In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
> @@ -3139,6 +3146,7 @@ void PruneThreadPlans();
// m_currently_handling_do_on_removals are true,
// Resume will only request a resume, using this
// flag to check.
+ lldb::RunDirection m_last_run_direction;
Feel free to initialize this here so we don't have to change the constructor:
lldb::RunDirection m_last_run_direction = lldb::eRunForward;
In lldb/include/lldb/lldb-enumerations.h <#99736 (comment)>:
> +/// Execution directions
+enum RunDirection { eRunForward, eRunReverse };
+
If we don't add an overload to continue by adding SBProcess::ReverseContinue() then this should be kept internal and not in this header file. If you add a new SBProcess::Continue(lldb::RunDirection) overload then this is needed. I would prefer a dedicated SBProcess::ReverseContinue() function.
Can you explain why? It seems like that usage is going to be more verbose to no purpose, as I showed above.
In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
> @@ -1129,10 +1129,15 @@ class Process : public std::enable_shared_from_this<Process>,
/// \see Thread:Resume()
/// \see Thread:Step()
/// \see Thread:Suspend()
- virtual Status DoResume() {
+ virtual Status DoResume(lldb::RunDirection direction) {
I would rather have a new DoResumeReverse() instead of changing this virtual API. Many modified files in this patch are a result of the process plugins having to add support for not supporting reverse resumes.
I also asked for this change in the review. Ever other API that does resume takes this parameter, its odd and asymmetric not to do so.
…
In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h <#99736 (comment)>:
> @@ -90,7 +90,7 @@ class ProcessKDP : public lldb_private::Process {
// Process Control
lldb_private::Status WillResume() override;
- lldb_private::Status DoResume() override;
+ lldb_private::Status DoResume(lldb::RunDirection direction) override;
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <#99736 (comment)>:
> @@ -203,11 +203,17 @@ ProcessWindows::DoAttachToProcessWithID(lldb::pid_t pid,
return error;
}
-Status ProcessWindows::DoResume() {
+Status ProcessWindows::DoResume(RunDirection direction) {
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp <#99736 (comment)>:
> @@ -402,9 +402,16 @@ lldb_private::DynamicLoader *ProcessKDP::GetDynamicLoader() {
Status ProcessKDP::WillResume() { return Status(); }
-Status ProcessKDP::DoResume() {
+Status ProcessKDP::DoResume(RunDirection direction) {
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <#99736 (comment)>:
> Log *log = GetLog(WindowsLog::Process);
llvm::sys::ScopedLock lock(m_mutex);
Status error;
+ if (direction == RunDirection::eRunReverse) {
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h <#99736 (comment)>:
> @@ -52,7 +52,7 @@ class ProcessWindows : public Process, public ProcessDebugger {
Status DoAttachToProcessWithID(
lldb::pid_t pid,
const lldb_private::ProcessAttachInfo &attach_info) override;
- Status DoResume() override;
+ Status DoResume(lldb::RunDirection direction) override;
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h <#99736 (comment)>:
> @@ -111,7 +111,7 @@ class ProcessGDBRemote : public Process,
// Process Control
Status WillResume() override;
- Status DoResume() override;
+ Status DoResume(lldb::RunDirection direction) override;
We should use DoResumeReverse() in Process.h and this change will add a new interface definition.
In lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp <#99736 (comment)>:
> @@ -182,10 +182,17 @@ void ScriptedProcess::DidResume() {
m_pid = GetInterface().GetProcessID();
}
-Status ScriptedProcess::DoResume() {
+Status ScriptedProcess::DoResume(RunDirection direction) {
We should use DoResumeReverse() in Process.h and this change won't need to happen.
In lldb/source/Plugins/Process/scripted/ScriptedProcess.h <#99736 (comment)>:
> @@ -52,7 +52,7 @@ class ScriptedProcess : public Process {
void DidResume() override;
- Status DoResume() override;
+ Status DoResume(lldb::RunDirection direction) override;
We should use DoResumeReverse() in Process.h and this change won't need to happen.
—
Reply to this email directly, view it on GitHub <#99736 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW6GKSTRDQ44D76RPE3ZN2CKDAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJUGQ4TMNBXGM>.
You are receiving this because you are on a team that was mentioned.
|
|
More generally, I think it will be more natural if reverse and forward continuations to be as much as possible "the exact same execution control machinery with a direction" not separate "forward" and "reverse" facilities. So I'd rather we not start off separating them artificially.
Jim
… On Jul 23, 2024, at 10:02 AM, Jim Ingham ***@***.***> wrote:
>
>
>
>> On Jul 23, 2024, at 9:47 AM, Greg Clayton ***@***.***> wrote:
>>
>>
>> @clayborg requested changes on this pull request.
>>
>> In lldb/include/lldb/API/SBProcess.h <#99736 (comment)>:
>>
>> > - lldb::SBError Continue();
>> + lldb::SBError Continue(RunDirection direction = RunDirection::eRunForward);
>> Our public API has rules:
>>
>> can't change any existing API call, you can add an overload, but you can't change any public APIs that are already there. Anything in the lldb namespace can't be changed.
>> no virtual functions
>> can't change any ivars or the size of the object
>> That being said, I would rather have a:
>>
>> lldb::SBError ReverseContinue();
>> call than have everyone using the Continue API to say wether they want to go forward or in reverse.
>>
>> In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
>>
>> > - Status Resume();
>> + Status Resume(lldb::RunDirection direction = lldb::eRunForward);
>> internal APIs, anything not in the lldb namespace can be changed. So this is ok. Though I personally would like to see a:
>>
>> Status ReverseResume();
>> I am open to feedback from other here as my mind can easily be changed.
>>
> You just have to omit the default argument, and leave the 0 argument form in place. That will have the effect you want, all previous code will use the forward continue, and new code that wants to be explicit can pass the argument.
>
> I asked for this change, because it seems like where you are going to use it you will have some variable telling yourself which direction you are going, and so if there's an argument, you will find yourself writing:
>
> my_process.Continue(the_direction_parameter_i_already_had)
>
> as opposed to
>
> if the_direction_parameter_i_already_had == lldb::eRunForward:
> my_process.Continue()
> else:
> my_process.ReverseContinue()
>
> The former seemed much nicer to me.
>
>
>>
>> In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
>>
>> > @@ -3139,6 +3146,7 @@ void PruneThreadPlans();
>> // m_currently_handling_do_on_removals are true,
>> // Resume will only request a resume, using this
>> // flag to check.
>> + lldb::RunDirection m_last_run_direction;
>> Feel free to initialize this here so we don't have to change the constructor:
>>
>> lldb::RunDirection m_last_run_direction = lldb::eRunForward;
>> In lldb/include/lldb/lldb-enumerations.h <#99736 (comment)>:
>>
>> > +/// Execution directions
>> +enum RunDirection { eRunForward, eRunReverse };
>> +
>> If we don't add an overload to continue by adding SBProcess::ReverseContinue() then this should be kept internal and not in this header file. If you add a new SBProcess::Continue(lldb::RunDirection) overload then this is needed. I would prefer a dedicated SBProcess::ReverseContinue() function.
>>
>
> Can you explain why? It seems like that usage is going to be more verbose to no purpose, as I showed above.
>
>>
>> In lldb/include/lldb/Target/Process.h <#99736 (comment)>:
>>
>> > @@ -1129,10 +1129,15 @@ class Process : public std::enable_shared_from_this<Process>,
>> /// \see Thread:Resume()
>> /// \see Thread:Step()
>> /// \see Thread:Suspend()
>> - virtual Status DoResume() {
>> + virtual Status DoResume(lldb::RunDirection direction) {
>> I would rather have a new DoResumeReverse() instead of changing this virtual API. Many modified files in this patch are a result of the process plugins having to add support for not supporting reverse resumes.
>>
> I also asked for this change in the review. Ever other API that does resume takes this parameter, its odd and asymmetric not to do so.
>
>>
>> In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h <#99736 (comment)>:
>>
>> > @@ -90,7 +90,7 @@ class ProcessKDP : public lldb_private::Process {
>> // Process Control
>> lldb_private::Status WillResume() override;
>>
>> - lldb_private::Status DoResume() override;
>> + lldb_private::Status DoResume(lldb::RunDirection direction) override;
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <#99736 (comment)>:
>>
>> > @@ -203,11 +203,17 @@ ProcessWindows::DoAttachToProcessWithID(lldb::pid_t pid,
>> return error;
>> }
>>
>> -Status ProcessWindows::DoResume() {
>> +Status ProcessWindows::DoResume(RunDirection direction) {
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp <#99736 (comment)>:
>>
>> > @@ -402,9 +402,16 @@ lldb_private::DynamicLoader *ProcessKDP::GetDynamicLoader() {
>>
>> Status ProcessKDP::WillResume() { return Status(); }
>>
>> -Status ProcessKDP::DoResume() {
>> +Status ProcessKDP::DoResume(RunDirection direction) {
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <#99736 (comment)>:
>>
>> > Log *log = GetLog(WindowsLog::Process);
>> llvm::sys::ScopedLock lock(m_mutex);
>> Status error;
>>
>> + if (direction == RunDirection::eRunReverse) {
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h <#99736 (comment)>:
>>
>> > @@ -52,7 +52,7 @@ class ProcessWindows : public Process, public ProcessDebugger {
>> Status DoAttachToProcessWithID(
>> lldb::pid_t pid,
>> const lldb_private::ProcessAttachInfo &attach_info) override;
>> - Status DoResume() override;
>> + Status DoResume(lldb::RunDirection direction) override;
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h <#99736 (comment)>:
>>
>> > @@ -111,7 +111,7 @@ class ProcessGDBRemote : public Process,
>> // Process Control
>> Status WillResume() override;
>>
>> - Status DoResume() override;
>> + Status DoResume(lldb::RunDirection direction) override;
>> We should use DoResumeReverse() in Process.h and this change will add a new interface definition.
>>
>> In lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp <#99736 (comment)>:
>>
>> > @@ -182,10 +182,17 @@ void ScriptedProcess::DidResume() {
>> m_pid = GetInterface().GetProcessID();
>> }
>>
>> -Status ScriptedProcess::DoResume() {
>> +Status ScriptedProcess::DoResume(RunDirection direction) {
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> In lldb/source/Plugins/Process/scripted/ScriptedProcess.h <#99736 (comment)>:
>>
>> > @@ -52,7 +52,7 @@ class ScriptedProcess : public Process {
>>
>> void DidResume() override;
>>
>> - Status DoResume() override;
>> + Status DoResume(lldb::RunDirection direction) override;
>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>
>> —
>> Reply to this email directly, view it on GitHub <#99736 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW6GKSTRDQ44D76RPE3ZN2CKDAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJUGQ4TMNBXGM>.
>> You are receiving this because you are on a team that was mentioned.
>>
>
>
|
rocallahan
force-pushed
the
reverse-continue
branch
from
8d23087 to
0269307
Compare
|
BTW, another reason I want to focus here on direction and not on explicitly stating "reverse continue" and "actual resume" is that a lot of "forward" motion in a debug session with reverse debugging capability is actually going to be going "forward" but using the "reverse" machinery, right? One pretty common workflow will be:
Most of the execution control logic should just know "forward" and "backwards", but then whether those are implemented by using the reverse debugging machinery or by allowing the program to run for realz will be know deeper in the execution control machinery - most likely governed by the thread plans. |
Actually LLDB is (and should be) oblivious to whether the gdbserver is running the program "for realz" or not. When using rr, we currently never run the program "for realz" while the user is in a debugging session. The workflow is: a) record a program run and save the recording to disk b) replay the run with the debugger attached. During b), both forward and reverse execution are "on rails", only observing what happened during recording. (Ok, ignoring user-requested function calls made through the debugger for now.) UndoDB and gdb's reverse execution are a bit different. They do support the workflow you're thinking of, where the program is actually "live" while you explore history in the past. But the distinction between "replaying through history" and "runinng live" is never surfaced in the gdbserver protocol and from the debugger's point of view I don't think it should matter. |
|
@rocallahan Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
|
|
Looks like |
|
I don't know what your normal protocol is but maybe you should revert it while I debug the failure? The last successful CI run in this PR was from September 25. |
|
I'll revert it! |
augusto2112
added a commit
that referenced
this pull request
Oct 10, 2024
This reverts commit d5e1de6.
|
I reverted both this commit and also b77fdf5, so you'll need to add that change to your commit before merging again. |
jasonmolenda
pushed a commit
that referenced
this pull request
Oct 10, 2024
This commit only adds support for the `SBProcess::ReverseContinue()` API. A user-accessible command for this will follow in a later commit. This feature depends on a gdbserver implementation (e.g. `rr`) providing support for the `bc` and `bs` packets. `lldb-server` does not support those packets, and there is no plan to change that. So, for testing purposes, `lldbreverse.py` wraps `lldb-server` with a Python implementation of *very limited* record-and-replay functionality for use by *tests only*. The majority of this PR is test infrastructure (about 700 of the 950 lines added).
jasonmolenda
added a commit
that referenced
this pull request
Oct 10, 2024
Reverting this again; I added a commit which added @skipIfDarwin markers to the TestReverseContinueBreakpoints.py and TestReverseContinueNotSupported.py API tests, which use lldb-server in gdbserver mode which does not work on Darwin. But the aarch64 ubuntu bot reported a failure on TestReverseContinueBreakpoints.py, https://lab.llvm.org/buildbot/#/builders/59/builds/6397 File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py", line 63, in test_reverse_continue_skip_breakpoint self.reverse_continue_skip_breakpoint_internal(async_mode=False) File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py", line 81, in reverse_continue_skip_breakpoint_internal self.expect( File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2372, in expect self.runCmd( File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1002, in runCmd self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Process should be stopped due to history boundary Error output: error: Process must be launched. This reverts commit 4f29756.
|
I tried re-applying this change and Jonas' fae7d68 to fix the Formatv call, and added @skipIfDarwin markers to the TestReverseContinueBreakpoints.py and TestReverseContinueNotSupported.py API tests because lldb-server is not supported in gdbserver mode on Darwin systems (c686eeb). But now it is faililng on an aarch64 Ubuntu bot, so I reverted all three patches again. |
|
Failed on Arm and AArch64: I'm looking at it now. |
|
This is something to do with evaluating With a normal session we see these libraries: And when we look up And use the first one. With the proxy server we see: And we find: I haven't got to exactly where it crashes but I see the debug server exit, so I suspect we're not meant to call this Anyway, that aside the problem is likely that we don't find the debug info files when using the proxy. I'm trying to find out why, I suspect some aspect of the real server isn't being passed on correctly. |
|
So on x86, either |
|
The only info I can get out of the call to We can reland this as x86 only, but give me a few days to see if I can figure this out. |
| if self.is_command(packet, "vCont", ";"): | ||
| if self.recording_enabled: | ||
| return self.continue_with_recording(packet) | ||
| snapshots = [] |
There was a problem hiding this comment.
This looks like at best a nop (this is a local variable not the self.snapshots), or a bug.
|
The problem is not to do with the debug symbols, or it being a "properly remote" connection due to the proxy and that messing up search paths. It's because evaluating an expression using JIT requires us to send a I know that if I stick Anyway, here is the log from a working non-proxied session, when I do This is the proxy's log for the same expression: It's as if there is a vCont missing between the QSaveRegisterState and QRestoreRegisterState. Is it possible that because this trigger breakpoint is hit when running in reverse, we're also trying to execute the expression "in reverse"? Getting very confused, and deciding that not doing anything is the best solution. Feels like the proxy's handling of vCont isn't at fault so much us running in reverse but then needing to forward execute to evaluate the expression. Because I'm able to evaluate the expression immediately prior to the I'm testing this on an AArch64 machine but I think you might be able to trigger this on X86 as well by making the breakpoint condition a function call instead of a variable. |
|
Also X86 may support the memory allocation packets and not need to call mmap to get JIT memory. So if making it a function call doesn't reproduce it, try hacking out support for |
|
I have created a new PR #112079 to continue work on this. |
So there is no LLDB-based daemon that implements the gdbserver protocol on Darwin systems? |
|
Answered in your new PR. |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
This commit only adds support for the `SBProcess::ReverseContinue()` API. A user-accessible command for this will follow in a later commit. This feature depends on a gdbserver implementation (e.g. `rr`) providing support for the `bc` and `bs` packets. `lldb-server` does not support those packets, and there is no plan to change that. So, for testing purposes, `lldbreverse.py` wraps `lldb-server` with a Python implementation of *very limited* record-and-replay functionality for use by *tests only*. The majority of this PR is test infrastructure (about 700 of the 950 lines added).
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
…)" This reverts commit d5e1de6.
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
This commit only adds support for the `SBProcess::ReverseContinue()` API. A user-accessible command for this will follow in a later commit. This feature depends on a gdbserver implementation (e.g. `rr`) providing support for the `bc` and `bs` packets. `lldb-server` does not support those packets, and there is no plan to change that. So, for testing purposes, `lldbreverse.py` wraps `lldb-server` with a Python implementation of *very limited* record-and-replay functionality for use by *tests only*. The majority of this PR is test infrastructure (about 700 of the 950 lines added).
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
…)" Reverting this again; I added a commit which added @skipIfDarwin markers to the TestReverseContinueBreakpoints.py and TestReverseContinueNotSupported.py API tests, which use lldb-server in gdbserver mode which does not work on Darwin. But the aarch64 ubuntu bot reported a failure on TestReverseContinueBreakpoints.py, https://lab.llvm.org/buildbot/#/builders/59/builds/6397 File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py", line 63, in test_reverse_continue_skip_breakpoint self.reverse_continue_skip_breakpoint_internal(async_mode=False) File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/functionalities/reverse-execution/TestReverseContinueBreakpoints.py", line 81, in reverse_continue_skip_breakpoint_internal self.expect( File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2372, in expect self.runCmd( File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1002, in runCmd self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Process should be stopped due to history boundary Error output: error: Process must be launched. This reverts commit 4f29756.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit only adds support for the
SBProcess::ReverseContinue()API. A user-accessible command for this will follow in a later commit.This feature depends on a gdbserver implementation (e.g.
rr) providing support for thebcandbspackets.lldb-serverdoes not support those packets, and there is no plan to change that. So, for testing purposes,lldbreverse.pywrapslldb-serverwith a Python implementation of very limited record-and-replay functionality for use by tests only.The majority of this PR is test infrastructure (about 700 of the 950 lines added).