Skip to content
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

GdbServer: Implement new netstream that can be interrupted #4223

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

GdbServer: Implement new netstream that can be interrupted #4223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de5cdae
GdbServer: Implement new netstream that can be interrupted
Sonicadvance1 Dec 18, 2024
95ee828
Netstream: Use a std::variant
Sonicadvance1 Jan 10, 2025
facec98
review
Sonicadvance1 Jan 13, 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
68 changes: 37 additions & 31 deletions Source/Tools/LinuxEmulation/LinuxSyscalls/GdbServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ namespace FEX {
#ifndef _WIN32
void GdbServer::Break(FEXCore::Core::InternalThreadState* Thread, int signal) {
std::lock_guard lk(sendMutex);
if (!CommsStream) {
if (!CommsStream.HasSocket()) {
return;
}

Expand All @@ -73,7 +73,7 @@ void GdbServer::Break(FEXCore::Core::InternalThreadState* Thread, int signal) {
CurrentDebuggingThread = ThreadObject->ThreadInfo.TID.load();

const auto str = fextl::fmt::format("T{:02x}thread:{:x};", signal, CurrentDebuggingThread);
SendPacket(*CommsStream, str);
SendPacket(str);
}

void GdbServer::WaitForThreadWakeup() {
Expand Down Expand Up @@ -178,7 +178,7 @@ static fextl::string encodeHex(std::string_view str) {
// Takes a serial stream and reads a single packet
// Un-escapes chars, checks the checksum and request a retransmit if it fails.
// Once the checksum is validated, it acknowledges and returns the packet in a string
fextl::string GdbServer::ReadPacket(std::iostream& stream) {
fextl::string GdbServer::ReadPacket() {
fextl::string packet {};

// The GDB "Remote Serial Protocal" was originally 7bit clean for use on serial ports.
Expand All @@ -190,9 +190,9 @@ fextl::string GdbServer::ReadPacket(std::iostream& stream) {
// where any $ or # in the packet body are escaped ('}' followed by the char XORed with 0x20)
// The checksum is a single unsigned byte sum of the data, hex encoded.

int c;
while ((c = stream.get()) > 0) {
switch (c) {
Utils::NetStream::ReturnGet c;
while ((c = CommsStream.get()).HasData()) {
switch (c.GetData()) {
case '$': // start of packet
if (packet.size() != 0) {
LogMan::Msg::EFmt("Dropping unexpected data: \"{}\"", packet);
Expand All @@ -203,15 +203,18 @@ fextl::string GdbServer::ReadPacket(std::iostream& stream) {
break;
case '}': // escape char
{
char escaped;
stream >> escaped;
packet.push_back(escaped ^ 0x20);
auto escaped = CommsStream.get();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a potential race condition here? An interrupt exactly after } would be caught here but silently be skipped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Now it will just permanently hang trying to receive more data, won't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless a hangup occurs, similar to previous behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix it? Not sure it makes sense to add the capability for interruptions and then only use it half of the time...

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no fix in this case. We need to wait until gdb has finished sending the message, otherwise the command stream is borked.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a scenario that can happen in practice or must other FEX code somehow ensure no interruptions in this specific part?

How do you currently plan to deal with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

gdb is likely to have already sent the message and its in our socket's buffer. The extreme case will be gdb has crashed which we notice as a hangup and early terminate.

When interrupted, it will continue waiting for its next message from gdb and handle whatever the interruption was about afterwards. There's nothing more to do.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing it, which I think relates back to my unanswered questions at the end of #4223 (comment) . I need to see an actual rundown of the problem scenario(s) you're addressing. It's unclear where interruptions happen and why, and which elements of this PR are mandated by GDB and which ones are pending on implementation choices.

Normally I just wouldn't expect async-aware code to be able to enter a permanently hanging state. It looks like a design flaw in the current implementation, but I can't pin it down more specifically without more context.

if (escaped.HasData()) {
packet.push_back(escaped.GetData() ^ 0x20);
} else {
LogMan::Msg::EFmt("Received Invalid escape char: ${}", packet);
}
break;
}
case '#': // end of packet
{
char hexString[3] = {0, 0, 0};
stream.read(hexString, 2);
CommsStream.read(hexString, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to the above, this doesn't seem to handle hangup events properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

int expected_checksum = std::strtoul(hexString, nullptr, 16);

if (calculateChecksum(packet) == expected_checksum) {
Expand All @@ -221,7 +224,7 @@ fextl::string GdbServer::ReadPacket(std::iostream& stream) {
}
break;
}
default: packet.push_back((char)c); break;
default: packet.push_back(c.GetData()); break;
}
}

Expand All @@ -248,22 +251,22 @@ static fextl::string escapePacket(const fextl::string& packet) {
return ss.str();
}

void GdbServer::SendPacket(std::ostream& stream, const fextl::string& packet) {
void GdbServer::SendPacket(const fextl::string& packet) {
const auto escaped = escapePacket(packet);
const auto str = fextl::fmt::format("${}#{:02x}", escaped, calculateChecksum(escaped));

stream << str << std::flush;
CommsStream.SendPacket(str);
}

void GdbServer::SendACK(std::ostream& stream, bool NACK) {
void GdbServer::SendACK(bool NACK) {
if (NoAckMode) {
return;
}

if (NACK) {
stream << "-" << std::flush;
CommsStream.SendPacket("-");
} else {
stream << "+" << std::flush;
CommsStream.SendPacket("+");
}

if (SettingNoAckMode) {
Expand Down Expand Up @@ -1341,16 +1344,16 @@ GdbServer::HandledPacketType GdbServer::ProcessPacket(const fextl::string& packe
void GdbServer::SendPacketPair(const HandledPacketType& response) {
std::lock_guard lk(sendMutex);
if (response.TypeResponse == HandledPacketType::TYPE_ACK || response.TypeResponse == HandledPacketType::TYPE_ONLYACK) {
SendACK(*CommsStream, false);
SendACK(false);
} else if (response.TypeResponse == HandledPacketType::TYPE_NACK || response.TypeResponse == HandledPacketType::TYPE_ONLYNACK) {
SendACK(*CommsStream, true);
SendACK(true);
}

if (response.TypeResponse == HandledPacketType::TYPE_UNKNOWN) {
SendPacket(*CommsStream, "");
SendPacket("");
} else if (response.TypeResponse != HandledPacketType::TYPE_ONLYNACK && response.TypeResponse != HandledPacketType::TYPE_ONLYACK &&
response.TypeResponse != HandledPacketType::TYPE_NONE) {
SendPacket(*CommsStream, response.Response);
SendPacket(response.Response);
}
}

Expand All @@ -1362,7 +1365,7 @@ GdbServer::WaitForConnectionResult GdbServer::WaitForConnection() {
int Result = ppoll(&PollFD, 1, nullptr, nullptr);
if (Result > 0) {
if (PollFD.revents & POLLIN) {
CommsStream = OpenSocket();
OpenSocket();
return WaitForConnectionResult::CONNECTION;
} else if (PollFD.revents & (POLLHUP | POLLERR | POLLNVAL)) {
// Listen socket error or shutting down
Expand Down Expand Up @@ -1393,12 +1396,11 @@ void GdbServer::GdbServerLoop() {
HandledPacketType response {};

// Outer server loop. Handles packet start, ACK/NAK and break

int c;
while ((c = CommsStream->get()) >= 0) {
switch (c) {
Utils::NetStream::ReturnGet c;
while (!CoreShuttingDown.load() && (c = CommsStream.get()).HasData()) {
switch (c.GetData()) {
case '$': {
auto packet = ReadPacket(*CommsStream);
auto packet = ReadPacket();
response = ProcessPacket(packet);
SendPacketPair(response);
if (response.TypeResponse == HandledPacketType::TYPE_UNKNOWN) {
Expand All @@ -1413,7 +1415,7 @@ void GdbServer::GdbServerLoop() {
// NAK, Resend requested
{
std::lock_guard lk(sendMutex);
SendPacket(*CommsStream, response.Response);
SendPacket(response.Response);
}
break;
case '\x03': { // ASCII EOT
Expand All @@ -1426,13 +1428,17 @@ void GdbServer::GdbServerLoop() {
SendPacketPair({std::move(str), HandledPacketType::TYPE_ACK});
break;
}
default: LogMan::Msg::DFmt("GdbServer: Unexpected byte {} ({:02x})", static_cast<char>(c), c);
default: LogMan::Msg::DFmt("GdbServer: Unexpected byte {} ({:02x})", c.GetData(), c.GetData());
}
}

if (c.HasHangup()) {
Copy link
Member

Choose a reason for hiding this comment

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

The control flow has become weird here (maybe since the latest changes?).

Why do we skip the InvalidateSocket call when we're receiving a Hangup?

Why is there still an outer while (!CoreShuttingDown.load()) { loop? I don't see how that outer loop would ever run more than one iteration, since we reach this line only when a Hangup is received or CoreShuttingDown==true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that was a bug introduced due to the refactoring request to delete an outer loop. Added the loop back because it's required to allow gdb connections to disconnect and reconnect at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

There's three levels of nesting loops now, two of them on the same variable...

  while (!CoreShuttingDown.load()) {
    // ...

    while (!CoreShuttingDown.load()) {
      // ...
      while ((c = CommsStream.get()).HasData()) {

break;
}

{
std::lock_guard lk(sendMutex);
CommsStream.reset();
CommsStream.InvalidateSocket();
}
}

Expand Down Expand Up @@ -1504,14 +1510,14 @@ void GdbServer::CloseListenSocket() {
unlink(GdbUnixSocketPath.c_str());
}

fextl::unique_ptr<std::iostream> GdbServer::OpenSocket() {
void GdbServer::OpenSocket() {
// Block until a connection arrives
struct sockaddr_storage their_addr {};
socklen_t addr_size {};

int new_fd = accept(ListenSocket, (struct sockaddr*)&their_addr, &addr_size);

return fextl::make_unique<FEXCore::Utils::NetStream>(new_fd);
CommsStream.OpenSocket(new_fd);
}

#endif
Expand Down
12 changes: 6 additions & 6 deletions Source/Tools/LinuxEmulation/LinuxSyscalls/GdbServer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ tags: glue|gdbserver
#include <FEXCore/fextl/string.h>

#include <atomic>
#include <istream>
#include <memory>
#include <mutex>
#include <stdint.h>

#include "LinuxSyscalls/NetStream.h"
#include "LinuxSyscalls/SignalDelegator.h"

namespace FEX {
Expand All @@ -45,12 +45,12 @@ class GdbServer {
ERROR,
};
WaitForConnectionResult WaitForConnection();
fextl::unique_ptr<std::iostream> OpenSocket();
void OpenSocket();
void StartThread();
fextl::string ReadPacket(std::iostream& stream);
void SendPacket(std::ostream& stream, const fextl::string& packet);
fextl::string ReadPacket();
void SendPacket(const fextl::string& packet);

void SendACK(std::ostream& stream, bool NACK);
void SendACK(bool NACK);

Event ThreadBreakEvent {};
void WaitForThreadWakeup();
Expand Down Expand Up @@ -147,7 +147,7 @@ class GdbServer {
FEX::HLE::SyscallHandler* const SyscallHandler;
FEX::HLE::SignalDelegator* SignalDelegation;
fextl::unique_ptr<FEXCore::Threads::Thread> gdbServerThread;
fextl::unique_ptr<std::iostream> CommsStream;
FEX::Utils::NetStream CommsStream;
std::mutex sendMutex;
bool SettingNoAckMode {false};
bool NoAckMode {false};
Expand Down
Loading
Loading