-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
base: llvm-server-plugins
Are you sure you want to change the base?
Conversation
@@ -929,21 +930,22 @@ llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) { | |||
packet.PutStringAsRawHex8(arg.ref()); | |||
} | |||
|
|||
StringExtractorGDBRemote response; | |||
if (SendPacketAndWaitForResponse(packet.GetString(), response) != | |||
StringExtractorGDBRemote vrun_response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to create a separate vrun_response here. We can just pass down the response we take as a parameter to LaunchProcess
.
return false; | ||
|
||
char first_char = m_packet.empty() ? '\0' : m_packet[0]; | ||
return first_char == 'T' || first_char == 'S'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add all the packets listed here (e.g. W
, X
): https://sourceware.org/gdb/current/onlinedocs/gdb.html/Stop-Reply-Packets.html#Stop-Reply-Packets
self.select_cpu() | ||
self.expect( | ||
"breakpoint list --internal", | ||
substrs=["gpu_first_stop"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to expand this check a bit to show that we have hit the breakpoint
Please run clang-format on this change to make sure it is formatted correctly. |
fd10120
to
7381246
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -916,7 +916,8 @@ lldb::pid_t GDBRemoteCommunicationClient::GetCurrentProcessID(bool allow_lazy) { | |||
return LLDB_INVALID_PROCESS_ID; | |||
} | |||
|
|||
llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) { |
There was a problem hiding this comment.
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.
bool StringExtractorGDBRemote::IsStopReply() const { | ||
if (!IsNormalResponse()) | ||
return false; | ||
|
||
char first_char = m_packet.empty() ? '\0' : m_packet[0]; | ||
return first_char == 'T' || first_char == 'S' || first_char == 'W' || | ||
first_char == 'X' || first_char == 'w' || first_char == 'N' || | ||
first_char == 'O' || first_char == 'F'; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check for 'T' or 'S' only here. 'W' is for exited, 'X' is for terminated, 'w' is for thread exited, 'N' is for non stop debugging, and 'O' is for output and 'F' is for calling syscalls.
So for using this in a launch scenario, we want to only check for a few. Maybe modifying this to be something like:
FLAGS_ENUM(StopReplyMask) {
Signal = (1u << 0),
Exited = (1u << 1),
Terminated = (1u << 2),
Output = (1u << 3),
NoResumed = (1u << 4),
Syscall = (1u << 5),
Any = ((Syscall << 1) - 1u),
};
bool StringExtractorGDBRemote::IsStopReply(uint32_t mask) const {
if (!IsNormalResponse())
return false;
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;
}
7e8a4d5
to
c0c7db4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, looks good!
GPUActions were not handled on the first stop-reply packed that is returned when we launch a process.
Modified the
LaunchProcess
method to capture and return the first stop-reply packet from thevRun
command instead of discarding it. InProcessGDBRemote::DoLaunch
, we now use this captured launch response directly if it's a valid stop-reply packet, so GPU actions in the launch response are processed instead of ignored..NOTE: It's just fixing the launch case, we also need to follow up and fix the attach and connect cases.