]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
gdbserver: fix vFile:stat to actually use 'stat'
authorAndrew Burgess <aburgess@redhat.com>
Wed, 11 Jun 2025 14:04:38 +0000 (15:04 +0100)
committerAndrew Burgess <aburgess@redhat.com>
Tue, 17 Jun 2025 20:21:33 +0000 (21:21 +0100)
This commit continues the work of the previous two commits.

In the following commits I added the target_fileio_stat function, and
the target_ops::fileio_stat member function:

  * 08a115cc1c4 gdb: add target_fileio_stat, but no implementations yet
  * 3055e3d2f13 gdb: add GDB side target_ops::fileio_stat implementation
  * 6d45af96ea5 gdbserver: add gdbserver support for vFile::stat packet
  * 22836ca8859 gdb: check for multiple matching build-id files

Unfortunately I messed up, despite being called 'stat' these function
actually performed an 'lstat'.  The 'lstat' is the correct (required)
implementation, it's the naming that is wrong.

Additionally, to support remote targets, these commit added the
vFile::stat packet, which again, performed an 'lstat'.

In the previous two commits I changed the GDB code to replace 'stat'
with 'lstat' in the fileio function names.  I then added a new
vFile:lstat packet which GDB now uses instead of vFile:stat.

And that just leaves the vFile:stat packet which is, right now,
performing an 'lstat'.

Now, clearly when I wrote this code I fully intended for this packet
to perform an lstat, it's the lstat that I needed.  But now, I think,
we should "fix" vFile:stat to actually perform a 'stat'.

This is risky.  This is a change in remote protocol behaviour.

Reasons why this might be OK:

  - vFile:stat was only added in GDB 16, so it's not been "in the
    wild" for too long yet.  If we're quick, we might be able to "fix"
    this before anyone realises I messed up.

  - The documentation for vFile:stat is pretty vague.  It certainly
    doesn't explicitly say "this does an lstat".  Most implementers
    would (I think), given the name, start by assuming this should be
    a 'stat' (given the name).  Only if they ran the full GDB
    testsuite, or examined GDB's implementation, would they know to
    use lstat.

Reasons why this might not be OK:

  - Some other debug client could be connecting to gdbserver, sending
    vFile:stat and expecting to get lstat behaviour.  This would break
    after this patch.

  - Some other remote server might have implemented vFile:stat
    support, and either figured out, or copied, the lstat behaviour
    from gdbserver.  This remote server would technically be wrong
    after this commit, but as GDB no longer uses vFile:stat, then this
    will only become a problem if/when GDB or some other client starts
    to use vFile:stat in the future.

Given the vague documentation for vFile:stat, and that it was only
added in GDB 16, I think we should fix it now to perform a 'stat', and
that is what this commit does.

The change in behaviour is documented in the NEWS file.  I've improved
the vFile:stat documentation in the manual to better explain what is
expected from this packet, and I've extended the existing test to
cover vFile:stat.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
gdb/NEWS
gdb/doc/gdb.texinfo
gdb/testsuite/gdb.server/fileio-packets.exp
gdb/testsuite/gdb.server/fileio-packets.py
gdbserver/hostio.cc

index 1b5e1e89534d62bbd59b494243addcf29c70e96a..d180c49c834685618cf15f5d9270f99932111dd2 100644 (file)
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -195,6 +195,11 @@ qXfer:threads:read
   should print as the target ID of the thread, for example in the
   "info threads" command or when switching to the thread.
 
+vFile:stat
+  Previously, gdbserver incorrectly implemented this packet using
+  lstat rather than stat.  This has now been corrected.  The
+  documentation has also been clarified.
+
 * MI changes
 
 ** The =library-unloaded event now includes the 'ranges' field, which
index a564307053eccb83259c1d1d1e8cc7c4b5d69c60..6139bc33d2e4c2c79d6caa4df38b8f2369306bb2 100644 (file)
@@ -46730,12 +46730,16 @@ If an error occurs the return value is -1.  The format of the
 returned binary attachment is as described in @ref{struct stat}.
 
 @item vFile:stat: @var{filename}
-Get information about the file @var{filename} on the target.
-On success the information is returned as a binary attachment
-and the return value is the size of this attachment in bytes.
-If an error occurs the return value is -1.  The format of the
+Get information about the file @var{filename} on the target as if from
+a @samp{stat} call.  On success the information is returned as a binary
+attachment and the return value is the size of this attachment in
+bytes.  If an error occurs the return value is -1.  The format of the
 returned binary attachment is as described in @ref{struct stat}.
 
+If @var{filename} is a symbolic link, then the information returned is
+about the file the link refers to, this is inline with the @samp{stat}
+library call.
+
 @item vFile:lstat: @var{filename}
 Get information about the file @var{filename} on the target as if from
 an @samp{lstat} call.  On success the information is returned as a
@@ -46743,7 +46747,8 @@ binary attachment and the return value is the size of this attachment
 in bytes.  If an error occurs the return value is -1.  The format of
 the returned binary attachment is as described in @ref{struct stat}.
 
-If @var{filename} is a symbolic link, then this packet returns
+This packet is identical to @samp{vFile:stat}, except if
+@var{filename} is a symbolic link, then this packet returns
 information about the link itself, not the file that the link refers
 to, this is inline with the @samp{lstat} library call.
 
index d243e93c9e7c2abb117b8a7f32b2e822be711498..9435efdc1906a727aea0b421b349ef6901578ce2 100644 (file)
@@ -58,3 +58,9 @@ gdb_test "python check_lstat(\"$test_file_1\")" "PASS" \
 
 gdb_test "python check_lstat(\"$test_file_2\")" "PASS" \
     "check remote lstat works on a symbolic link"
+
+gdb_test "python check_stat(\"$test_file_1\")" "PASS" \
+    "check remote stat works on a normal file"
+
+gdb_test "python check_stat(\"$test_file_2\")" "PASS" \
+    "check remote stat works on a symbolic link"
index 53fb49e81670b31a9d71e2068485f367582cd386..293ae7e2aef4f6eb523411d7051afb7313b27a83 100644 (file)
@@ -114,6 +114,20 @@ def remote_lstat(filename):
     return stat
 
 
+# Perform a stat of remote file FILENAME, and create a dictionary of
+# the results, the keys are the fields of the stat structure.
+def remote_stat(filename):
+    conn = gdb.selected_inferior().connection
+    if not isinstance(conn, gdb.RemoteTargetConnection):
+        raise gdb.GdbError("connection is the wrong type")
+
+    filename_hex = hex_encode(filename)
+    reply = conn.send_packet("vFile:stat:%s" % filename_hex)
+
+    stat = decode_stat_reply(reply)
+    return stat
+
+
 # Convert a stat_result object to a dictionary that should match the
 # dictionary built from the remote protocol reply.
 def stat_result_to_dict(res):
@@ -154,6 +168,13 @@ def local_lstat(filename):
     return stat_result_to_dict(res)
 
 
+# Perform an lstat of local file FILENAME, and create a dictionary of
+# the results, the keys are the fields of the stat structure.
+def local_stat(filename):
+    res = os.stat(filename)
+    return stat_result_to_dict(res)
+
+
 # Perform a remote lstat using GDB, and a local lstat using os.lstat.
 # Compare the results to check they are the same.
 #
@@ -163,8 +184,24 @@ def check_lstat(filename):
     s1 = remote_lstat(filename)
     s2 = local_lstat(filename)
 
-    print(f"s1 = {s1}")
-    print(f"s2 = {s2}")
+    print(f"remote = {s1}")
+    print(f"local  = {s2}")
+
+    assert s1 == s2
+    print("PASS")
+
+
+# Perform a remote stat using GDB, and a local stat using os.stat.
+# Compare the results to check they are the same.
+#
+# For this test to work correctly, gdbserver, and GDB (where this
+# Python script is running), must see the same filesystem.
+def check_stat(filename):
+    s1 = remote_stat(filename)
+    s2 = local_stat(filename)
+
+    print(f"remote = {s1}")
+    print(f"local  = {s2}")
 
     assert s1 == s2
     print("PASS")
index 05f2c4e0b3387668a2d5b0c5024946abc7f1b9ea..8b4d74d5942c82f461b831b367d39acc04702477 100644 (file)
@@ -504,7 +504,7 @@ handle_stat (char *own_buf, int *new_packet_len)
       return;
     }
 
-  if (lstat (filename, &st) == -1)
+  if (stat (filename, &st) == -1)
     {
       hostio_error (own_buf);
       return;