Skip to content

GPUActions are ignored on first stop-reply packet fix #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: llvm-server-plugins
Choose a base branch
from
Open

GPUActions are ignored on first stop-reply packet fix #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2352eec
GPUActions are ignored on first stop-reply packet fix
Sep 16, 2025
19c3bb4
fixed comments
Sep 29, 2025
068f1fa
fixed test
Oct 8, 2025
bc65e24
fixed Jeffrey comment
Oct 14, 2025
f1293d0
added correct GPUActions identifer
Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lldb/include/lldb/Utility/StringExtractorGDBRemote.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,19 @@ class StringExtractorGDBRemote : public StringExtractor {

bool IsErrorResponse() const;

enum StopReplyMask : uint32_t {
Signal = (1u << 0),
Exited = (1u << 1),
Terminated = (1u << 2),
Output = (1u << 3),
NoResumed = (1u << 4),
Syscall = (1u << 5),
Any = ((Syscall << 1) - 1u),
};

bool IsStopReply() const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this overload (and it is not implemented). Just specify the default value in the declaration below.

bool IsStopReply(uint32_t mask) const;

// Returns zero if the packet isn't a EXX packet where XX are two hex digits.
// Otherwise the error encoded in XX is returned.
uint8_t GetError();
Expand Down
6 changes: 4 additions & 2 deletions lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,10 +371,12 @@ Status PlatformRemoteGDBServer::LaunchProcess(ProcessLaunchInfo &launch_info) {
Args args = launch_info.GetArguments();
if (FileSpec exe_file = launch_info.GetExecutableFile())
args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
if (llvm::Error err = m_gdb_client_up->LaunchProcess(args)) {
llvm::Expected<StringExtractorGDBRemote> response =
m_gdb_client_up->LaunchProcess(args);
if (!response) {
error = Status::FromErrorStringWithFormatv(
"Cannot launch '{0}': {1}", args.GetArgumentAtIndex(0),
llvm::fmt_consume(std::move(err)));
llvm::fmt_consume(response.takeError()));
return error;
}
}
Expand Down
12 changes: 7 additions & 5 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,10 +916,14 @@ lldb::pid_t GDBRemoteCommunicationClient::GetCurrentProcessID(bool allow_lazy) {
return LLDB_INVALID_PROCESS_ID;
}

llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to change the return value to:

llvm::Expected<StringExtractorGDBRemote> GDBRemoteCommunicationClient::LaunchProcess(const Args &args);

Then we don't need to pass the StringExtractorGDBRemote &response in as a parameter.

llvm::Expected<StringExtractorGDBRemote>
GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
if (!args.GetArgumentAtIndex(0))
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"Nothing to launch");

StringExtractorGDBRemote response;

// try vRun first
if (m_supports_vRun) {
StreamString packet;
Expand All @@ -929,7 +933,6 @@ llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
packet.PutStringAsRawHex8(arg.ref());
}

StringExtractorGDBRemote response;
if (SendPacketAndWaitForResponse(packet.GetString(), response) !=
PacketResult::Success)
return llvm::createStringError(llvm::inconvertibleErrorCode(),
Expand All @@ -942,7 +945,7 @@ llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
// FIXME: right now we just discard the packet and LLDB queries
// for stop reason again
if (!response.IsUnsupportedResponse())
return llvm::Error::success();
return response;

m_supports_vRun = false;
}
Expand All @@ -957,7 +960,6 @@ llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
packet.PutStringAsRawHex8(arg.value().ref());
}

StringExtractorGDBRemote response;
if (SendPacketAndWaitForResponse(packet.GetString(), response) !=
PacketResult::Success) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
Expand All @@ -972,7 +974,7 @@ llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) {
"Sending qLaunchSuccess packet failed");
}
if (response.IsOKResponse())
return llvm::Error::success();
return response;
if (response.GetChar() == 'E') {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
response.GetStringRef().substr(1));
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
///
/// \param[in] args
/// A list of program arguments. The first entry is the program being run.
llvm::Error LaunchProcess(const Args &args);
llvm::Expected<StringExtractorGDBRemote> LaunchProcess(const Args &args);

/// Sends a "QEnvironment:NAME=VALUE" packet that will build up the
/// environment that will get used when launching an application
Expand Down
13 changes: 8 additions & 5 deletions lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module,

// Send the environment and the program + arguments after we connect
m_gdb_comm.SendEnvironment(launch_info.GetEnvironment());

StringExtractorGDBRemote response;
{
// Scope for the scoped timeout object
GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
Expand All @@ -805,11 +805,14 @@ Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module,
Args args = launch_info.GetArguments();
if (FileSpec exe_file = launch_info.GetExecutableFile())
args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) {
llvm::Expected<StringExtractorGDBRemote> result =
m_gdb_comm.LaunchProcess(args);
if (!result) {
error = Status::FromErrorStringWithFormatv(
"Cannot launch '{0}': {1}", args.GetArgumentAtIndex(0),
llvm::fmt_consume(std::move(err)));
llvm::fmt_consume(result.takeError()));
} else {
response = std::move(*result);
SetID(m_gdb_comm.GetCurrentProcessID());
}
}
Expand All @@ -820,8 +823,8 @@ Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module,
return error;
}

StringExtractorGDBRemote response;
if (m_gdb_comm.GetStopReply(response)) {
if (response.IsStopReply(StringExtractorGDBRemote::Signal) ||
m_gdb_comm.GetStopReply(response)) {
SetLastStopPacket(response);

const ArchSpec &process_arch = m_gdb_comm.GetProcessArchitecture();
Expand Down
21 changes: 21 additions & 0 deletions lldb/source/Utility/StringExtractorGDBRemote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,27 @@ bool StringExtractorGDBRemote::IsErrorResponse() const {
isxdigit(m_packet[2]);
}

bool StringExtractorGDBRemote::IsStopReply(
uint32_t mask = StopReplyMask::Any) const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default values need to go on the declaration not the definition.

if (!IsNormalResponse())
return false;

char first_char = m_packet.empty() ? '\0' : m_packet[0];
if (mask & Signal && (first_char == 'T' || first_char == 'S'))
return true;
if (mask & Exited && (first_char == 'w' || first_char == 'W'))
return true;
if (mask & Terminated && first_char == 'X')
return true;
if (mask & Output && first_char == 'O')
return true;
if (mask & NoResumed && first_char == 'N')
return true;
if (mask & Syscall && first_char == 'F')
return true;
return false;
}

uint8_t StringExtractorGDBRemote::GetError() {
if (GetResponseType() == eError) {
SetFilePos(1);
Expand Down
12 changes: 12 additions & 0 deletions lldb/test/API/gpu/mock/basic/TestBasicMockGpuPlugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,15 @@ def test_mock_gpu_target_switch(self):

self.runCmd("target switch cpu")
self.assertTrue(self.dbg.GetSelectedTarget() == self.cpu_target)
def test_mock_gpu_first_breakpoint_hit(self):
"""
Test that we can hit the first breakpoint on the cpu target,
Making sure we handle the GPUActions on the first stop-reply.
"""
self.common_setup()

self.select_cpu()
self.expect(
"breakpoint list --internal",
patterns=[r"name = 'gpu_first_stop'.*hit count = 1"],
)
2 changes: 2 additions & 0 deletions lldb/test/API/gpu/mock/basic/hello_world.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ struct ShlibInfo {

ShlibInfo g_shlib_list = {"/tmp/a.out", nullptr};

int gpu_first_stop() { return puts(__FUNCTION__); }
int gpu_initialize() { return puts(__FUNCTION__); }
int gpu_shlib_load() { return puts(__FUNCTION__); }
int gpu_third_stop() { return puts(__FUNCTION__); }
Expand All @@ -16,6 +17,7 @@ int gpu_kernel() {
}

int main(int argc, const char **argv) {
gpu_first_stop();
// CPU BREAKPOINT - BEFORE INITIALIZE
gpu_initialize();
// CPU BREAKPOINT - AFTER INITIALIZE
Expand Down
23 changes: 18 additions & 5 deletions lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ using namespace lldb_private::lldb_server;
using namespace lldb_private::process_gdb_remote;

enum {
BreakpointIDInitialize = 1,
BreakpointIDShlibLoad = 2,
BreakpointIDThirdStop = 3,
BreakpointIDResumeAndWaitForResume = 4,
BreakpointIDWaitForStop = 5
BreakpointIDFirstStop = 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that we added a new BreakpointIDFirstStop but never used in this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. We don't really use it for our testing by as you mentioned below, I accidentally used BreakpointIDThirdStop rather than BreakpointIDFirstStop.

BreakpointIDInitialize = 2,
BreakpointIDShlibLoad = 3,
BreakpointIDThirdStop = 4,
BreakpointIDResumeAndWaitForResume = 5,
BreakpointIDWaitForStop = 6
};

LLDBServerPluginMockGPU::LLDBServerPluginMockGPU(
Expand Down Expand Up @@ -152,6 +153,18 @@ std::optional<GPUPluginConnectionInfo> LLDBServerPluginMockGPU::CreateConnection

std::optional<GPUActions> LLDBServerPluginMockGPU::NativeProcessIsStopping() {
NativeProcessProtocol *native_process = m_native_process.GetCurrentProcess();
static bool first_time = true;
Log *log = GetLog(GDBRLog::Plugin);
LLDB_LOGF(log, "%d stop id ", native_process->GetStopID());
if (first_time) {
first_time = false;
GPUActions actions = GetNewGPUAction();
GPUBreakpointInfo bp;
bp.identifier = BreakpointIDFirstStop;
bp.name_info = {"a.out", "gpu_first_stop"};
actions.breakpoints.emplace_back(std::move(bp));
return actions;
}
// Show that we can return a valid GPUActions object from a stop event.
if (native_process->GetStopID() == 3) {
GPUActions actions = GetNewGPUAction();
Expand Down