Skip to content

[lldb] Only send "posix" error codes through the gdb-remote protocol #108170

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

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

labath
Copy link
Collaborator

@labath labath commented Sep 11, 2024

The other side has no way of telling which namespace do these codes belong to, so mashing them all together is not very helpful.

I'm mainly doing this to simplify some code in a pending patch https://github.com/llvm/llvm-project/pull/106774/files#r1752628604, and I've picked the posix error category semi-randomly. If we wanted to be serious about assigning meaning to these error codes, we should create a special error category for "gdb errors".

The other side has no way of telling which namespace do these codes
belong to, so mashing them all together is not very helpful.

I'm mainly doing this to simplify some code in a pending patch
<https://github.com/llvm/llvm-project/pull/106774/files#r1752628604>,
and I've picked the posix error category semi-randomly. If we wanted to
be serious about assigning meaning to these error codes, we should
create a special error category for "gdb errors".
@llvmbot
Copy link
Member

llvmbot commented Sep 11, 2024

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

The other side has no way of telling which namespace do these codes belong to, so mashing them all together is not very helpful.

I'm mainly doing this to simplify some code in a pending patch <https://github.com/llvm/llvm-project/pull/106774/files#r1752628604>, and I've picked the posix error category semi-randomly. If we wanted to be serious about assigning meaning to these error codes, we should create a special error category for "gdb errors".


Full diff: https://github.com/llvm/llvm-project/pull/108170.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp (+4-3)
  • (modified) lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp (+2-1)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
index 9b72cb00352821..d4aa90b2c7731a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -103,13 +103,14 @@ GDBRemoteCommunicationServer::SendErrorResponse(uint8_t err) {
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendErrorResponse(const Status &error) {
+  uint8_t code = error.GetType() == eErrorTypePOSIX ? error.GetError() : 0xff;
   if (m_send_error_strings) {
     lldb_private::StreamString packet;
-    packet.Printf("E%2.2x;", static_cast<uint8_t>(error.GetError()));
+    packet.Printf("E%2.2x;", code);
     packet.PutStringAsRawHex8(error.AsCString());
     return SendPacketNoLock(packet.GetString());
-  } else
-    return SendErrorResponse(error.GetError());
+  }
+  return SendErrorResponse(code);
 }
 
 GDBRemoteCommunication::PacketResult
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
index 69ca1720c04fc9..ba9ca6ea73e3be 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
@@ -12,6 +12,7 @@
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
 #include "lldb/Utility/Connection.h"
 #include "lldb/Utility/UnimplementedError.h"
+#include "lldb/lldb-enumerations.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
@@ -25,7 +26,7 @@ TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorNumber) {
 
 TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_Status) {
   MockServerWithMockConnection server;
-  Status status(0x42, lldb::eErrorTypeGeneric, "Test error message");
+  Status status(0x42, lldb::eErrorTypePOSIX, "Test error message");
   server.SendErrorResponse(status);
 
   EXPECT_THAT(

@DavidSpickett
Copy link
Collaborator

Does this impact #106950 at all?

@labath
Copy link
Collaborator Author

labath commented Sep 11, 2024

Does this impact #106950 at all?

It's related to it in the sense that the error codes we're sending are rubish, but it doesn't really "impact" it because the "host IO" packets use a different response format (Fxx,yy) vs (Exx). If we wanted to assign meaning to these errors, we'd need some sort of an error translation function, which we could then use from the host io code and also from this function.

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

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

LGTM

@labath labath merged commit ebeb56a into llvm:main Oct 11, 2024
9 checks passed
@labath labath deleted the err branch October 11, 2024 08:40
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…lvm#108170)

The other side has no way of telling which namespace do these codes
belong to, so mashing them all together is not very helpful.

I'm mainly doing this to simplify some code in a pending patch
<https://github.com/llvm/llvm-project/pull/106774/files#r1752628604>,
and I've picked the posix error category semi-randomly. If we wanted to
be serious about assigning meaning to these error codes, we should
create a special error category for "gdb errors".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants