[lldb] fix vFile:open, vFile:unlink error codes

[dlav-sc]

This patch makes gdb-server sends only GDB RSP supported error codes during vFile:open and vFile:unlink handling.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (dlav-sc)

Changes

This patch fixes a error code parsing of gdb remote protocol response messages to File-I/O command.

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+26-7)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 0297fe363f69e1..bce50eb4f8a907 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3064,22 +3064,41 @@ static int gdb_errno_to_system(int err) {
 
 static uint64_t ParseHostIOPacketResponse(StringExtractorGDBRemote &response,
                                           uint64_t fail_result, Status &error) {
+  // The packet is expected to have the following format:
+  // 'F<retcode>,<errno>'
+
   response.SetFilePos(0);
   if (response.GetChar() != 'F')
     return fail_result;
+
   int32_t result = response.GetS32(-2, 16);
   if (result == -2)
     return fail_result;
-  if (response.GetChar() == ',') {
-    int result_errno = gdb_errno_to_system(response.GetS32(-1, 16));
-    if (result_errno != -1)
-      error = Status(result_errno, eErrorTypePOSIX);
-    else
-      error = Status(-1, eErrorTypeGeneric);
-  } else
+
+  if (response.GetChar() != ',') {
     error.Clear();
+    return result;
+  }
+
+  // Response packet should contain a error code at the end. This code
+  // corresponds either to the gdb IOFile error code, or to the posix errno.
+  int32_t result_errno = response.GetS32(-1, 16);
+  if (result_errno == -1) {
+    error.SetError(-1, eErrorTypeGeneric);
+    return result;
+  }
+
+  int rsp_errno = gdb_errno_to_system(result_errno);
+  if (rsp_errno != -1) {
+    error.SetError(rsp_errno, eErrorTypePOSIX);
+    return result;
+  }
+
+  // Have received a system error that isn't described in gdb rsp protocol
+  error.SetError(result_errno, eErrorTypePOSIX);
   return result;
 }
+
 lldb::user_id_t
 GDBRemoteCommunicationClient::OpenFile(const lldb_private::FileSpec &file_spec,
                                        File::OpenOptions flags, mode_t mode,

[DavidSpickett]

And this error is what exactly, what did we previously do and what does this PR now make us do?

I'm of course wondering if this can be tested too but let's be clear what's being fixed first and I'll see whether it can be added to some existing tests.

[dlav-sc]

And this error is what exactly, what did we previously do and what does this PR now make us do?

I'm of course wondering if this can be tested too but let's be clear what's being fixed first and I'll see whether it can be added to some existing tests.

In my case lldb-server can't open file on a remote machine. Lldb client sends vFile:open message on the server, server tries to open a file, but receives ETXTBSY error. Lldb-server just checks that errno corresponding to some posix error
(if (code.category() == std::system_category())) and if it so sends reply message with that posix error code, like it was with ETXTBSY. Then client receives this reply message, it tries to map errno on gdb rsp supported I/O-File error codes (which are in fact a limited set of posix errors that can occur then working with file). The problem is that gdb rsp doesn't has ETXTBSY in the list of supported errors https://sourceware.org/gdb/current/onlinedocs/gdb.html/Errno-Values.html#Errno-Values, so in this cases the error is considered as unknown.
Currently, I am working on the patch, that in some cases fix ETXTBSY error, and at the beginning it was hard for me to understand what the problem is, so I've decided try to make error messages more clear.

I have an idea to just add ETXTBSY error code into gdb rsp and then add it here, but maybe there is another errors like ETXTBSY that can appear in I/O-File messages, so I have decided to handle it in such way.

[DavidSpickett]

Please update the PR description with an explanation of the bug you are fixing, along the lines of the comment you wrote previously.

[DavidSpickett]

If the remote server is connect to a posix machine then yes it'll be posix but we don't know that. I think if we don't recognise it as one of the gdb errnos, it should be eErrorTypeGeneric.

[dlav-sc]

I've taken one more look at the documentation, and there are two options that seem likely now:

1. According to the gdb rsp, error values can only be from the gdb internal list (https://sourceware.org/gdb/current/onlinedocs/gdb.html/Errno-Values.html#Errno-Values). So, in the case of ETXTBSY, the response packet value cannot contain 26 as the error code, and it must be 9999 instead, which corresponds to the unknown error. In this case, the error is most likely on the lldb server side.

2. Lldb documentation (see lldb/docs/lldb-platform-packets.txt) states that vFile:open, vFile:close, vFile:symlink, vFile:unlink, vFile:mode, and vFile:size reply packets contain errno. I think there was an attempt to extend gdb rsp so that a respond packet could contain any posix error, instead of the limited set suggested by the rsp. I think this can be possible, because as far as I know, the lldb server does not currently run on systems that are not compatible with posix.

Please let me know what you think of this matter.

[DavidSpickett]

Yes it is a bit vague but going by https://lldb.llvm.org/resources/lldbgdbremote.html#vfile-open "the errno" doesn't say to me "the posix errno" necessarily. You are right that for lldb-server specifically, it would defacto mean the posix errno, but there is also Windows to consider.

But the code you are editing here is the client side (lldb). The client could be connected to a debug server running on anything.

So I don't think you can mark the unknown values eErrorTypePOSIX because at this point we don't know that the other end is posix. So that's why I'd go with eErrorTypeGeneric.

(I suspect this doesn't really matter in the end, we probably just print the error number the same way either way)

The point of your patch is to include the number instead of overwriting it with -1, and that is fine I understand why that's useful.

[jasonmolenda]

One background info: lldb/docs/lldb-platform-packets.txt was written by me as I was reading the lldb/debugserver/lldb-server implementations of these packets, and trying to cross-reference with the official gdb remote serial protocol documentation. Any disagreement between lldb-platform-packets.txt and the official docs should be assumed to be a mistake on my part, probably reflecting how lldb was currently handling things. As you say, if we wanted to relay a new error type, we'd need to get gdb to add that to the "errno" table in their docs. Otherwise the only safe error type we can use is EUNKNOWN.

[DavidSpickett]

True, this would be a divergence.

I think hiding the original number is pretty annoying though, and my understanding is that's the motivation here. Even if you have access to the remote side, getting logging out of lldb-server is a pain.

We added something for adding string errors to certain packets a while ago, I think launch uses it. If I can find that, perhaps we can use it to append the original error number instead.

[dlav-sc]

GDB RSP supports 2 types of error response packets: E xx and E.errtext (https://sourceware.org/gdb/current/onlinedocs/gdb.html/Standard-Replies.html#Standard-Replies), so it looks like the ability to send error strings should be available for all targets by default.

Could you consider a couple of my questions about the matter, please?

1. Taking into account what I've said above, why do we need QEnableErrorStrings?
2. Is there any function like SendErrorRespond, which takes a error string and forms E.errtext packet? To be honest, I couldn't find something like this.

[jasonmolenda]

Sorry for the delay in replying. I didn't realize that gdb has implemented this differently than lldb did - I thought https://reviews.llvm.org/D34945 was pulling over a gdb-originated feature. It looks like gdb needs to send error-message in the qSupported list at the start of the debug session to get those error strings (v. https://sourceware.org/gdb/current/onlinedocs/gdb.html/General-Query-Packets.html#error_002dmessage ) although looking a little more closely, I think this feature was only added to the gdb remote serial protocol in May of this year? lldb's QEnableErrorStrings was added in 2017. v. https://sourceware.org/pipermail/gdb-patches/2024-May/209040.html

[jasonmolenda]

We should have lldb send error-message in our QSupported list, and accept this style of error reporting from the remote stub, but that's clearly an entirely different topic than the one being discussed here.

[jasonmolenda]

Note that the error strings, as currently used by QEnableErrorStrings, are only relevant for gdb protocol errors (packets of format Exx). The "host IO" packets discussed here use a different format for reporting errors, and we don't currently have a way to include error strings in those.

Ah, excellent point, I'd forgotten that the vFile packets have a special error reporting mechanism that's entirely different. https://sourceware.org/gdb/current/onlinedocs/gdb.html/Host-I_002fO-Packets.html#Host-I_002fO-Packets

[dlav-sc]

I think this feature was only added to the gdb remote serial protocol in May of this year?

Yeah, looks like you're right, I didn't notice this.

Thanks for your reply.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dlav-sc]

@DavidSpickett @jasonmolenda I've made a fix on the server side, could you take a look, please?

I also did the same thing with vFile:unlink, because it is a standard GDB RSP request like vFile:open, so I guess it should follow GDB RSP rules. Custom requests like vFile:MD5, vFile:size, etc in their turn I leave as they are.

[jasonmolenda]

Sorry for the delay in reviewing, this looks good to me, thanks for doing this.

[DavidSpickett]

LGTM

In theory we could test this but it'd be platform specific and error injection is always tricky, the change is straightforward, so let's not worry about it.

[DavidSpickett]

Feels like we diverted you from your original problem though, which was the remote taking the informative errno and changing it to some meaningless value.

If you've solved your problem then no obligation to do any more but if you do want to look into the other reporting methods that would be welcome.

[jasonmolenda]

Feels like we diverted you from your original problem though, which was the remote taking the informative errno and changing it to some meaningless value.

It should do that though, shouldn't it? We look up the ERRNO constant we got in the GDBRemoteErrno table, sending back the constant values that the gdb RSP docs define, and if it's an errno not included in that table, we send back GDB_EUNKNOWN.

[DavidSpickett]

Yes working to the standard is good generally, but in the minority of cases where you do get a strange errno, GDB_EUNKNOWN is zero help and then you're working out the lldb-server gdbserver --log-channels etc. command to get it to print the real number.

So far that hasn't been annoying enough to enough people for anyone to add a side channel to send the unmodified errno.