Re: [PATCH v1] gdbserver: widen the stat fields returned by the vFile packets

[Abhay Kandpal <abhay@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 6673 bytes --]

Hi Eli,

On 03/09/26 21:30, Eli Zaretskii wrote:
>> From: Abhay Kandpal<abhay@linux.ibm.com>
>> Cc:aburgess@redhat.com,cel@linux.ibm.com,abhay.k@ibm.com,
>>   Abhay Kandpal<abhay@linux.ibm.com>
>> Date: Thu,  3 Sep 2026 09:05:55 -0500
>>
>>
>> gdb/
>> 	* NEWS: Mention the stat64 feature.
>> 	* doc/gdb.texinfo (General Query Packets): Document the stat64
>> 	feature.
>> 	(Host I/O Packets): Mention the wide reply format.
>> 	(struct stat64): New node.
>> 	* remote-fileio.c (remote_fileio_to_host_ulong8): New function.
>> 	(remote_fileio_wide_to_host_stat): New function.
>> 	* remote-fileio.h (remote_fileio_wide_to_host_stat): Declare.
>> 	* remote.c (PACKET_stat64_feature): New enum value.
>> 	(remote_features::stat64_feature): New method.
>> 	(remote_protocol_features): Add stat64.
>> 	(remote_target::remote_query_supported): Send stat64+.
>> 	(fileio_process_fstat_and_lstat_reply): Handle both formats.
>> 	(INIT_GDB_FILE): Add the stat64-feature packet command.
>> 	* testsuite/gdb.server/fileio-packets.py (decode_stat_reply):
>> 	Handle both the narrow and wide reply formats.
>>
>> gdbserver/
>> 	* hostio.cc (handle_fstat, handle_stat, handle_lstat): Send the
>> 	wide struct when the feature is negotiated.
>> 	* server.cc (handle_query): Handle and report stat64.
>> 	* server.h (client_state::stat64_feature): New member.
>>
>> gdbsupport/
>> 	* fileio.cc (host_to_fileio_stat_wide): New function.
>> 	(narrow_field, fileio_stat_wide_to_narrow): New functions.
>> 	* fileio.h (struct fio_stat_wide): New struct.
>> 	(bigendian_to_host): New function.
>> 	(host_to_fileio_stat_wide, fileio_stat_wide_to_narrow): Declare.
>>
>> Bug:https://sourceware.org/bugzilla/show_bug.cgi?id=34567
>> ---
>> This patch is reg tested.
>>
>>   gdb/NEWS                                   |  12 +++
>>   gdb/doc/gdb.texinfo                        |  49 ++++++++++
>>   gdb/remote-fileio.c                        |  35 ++++++-
>>   gdb/remote-fileio.h                        |   4 +
>>   gdb/remote.c                               |  36 +++++--
>>   gdb/testsuite/gdb.server/fileio-packets.py |  59 +++++++-----
>>   gdbserver/hostio.cc                        | 106 +++++++++++++++------
>>   gdbserver/server.cc                        |   4 +-
>>   gdbserver/server.h                         |   5 +
>>   gdbsupport/fileio.cc                       |  70 ++++++++++++++
>>   gdbsupport/fileio.h                        |  54 ++++++++++-
>>   11 files changed, 368 insertions(+), 66 deletions(-)
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 222f38e3d52..de8346350ab 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,18 @@
>>   
>>   *** Changes since GDB 18
>>   
>> +* Changed remote packets
>> +
>> +stat64 in qSupported
>> +
>> +  The new stat64 feature within the qSupported packet allows GDB and
>> +  the stub to agree on a wider form of the stat structure returned by
>> +  the vFile:fstat, vFile:stat and vFile:lstat packets, in which st_dev,
>> +  st_ino, st_nlink, st_rdev and the three timestamps are 8 bytes rather
>> +  than 4.  The wider form is used only when both sides report the
>> +  feature; otherwise the existing format is used and values which do
>> +  not fit are truncated as before.
> This is okay, but does it mean that 32-bit timestamps mean that
> version will die after 2038?  If so, perhaps this should be mentioned
> in the NEWS entry?

It's 2106 rather than 2038. The narrow field is read as unsigned:
remote_fileio_to_host_time calls remote_fileio_to_host_uint, which uses
extract_unsigned_integer, so the range is 0 to 4294967295 seconds from
the epoch.  I'll mention this in the NEWS entry.

>
>> +@item stat64
>> +The remote stub supports the wider form of the stat structure returned
>> +by the @samp{vFile:fstat}, @samp{vFile:stat} and @samp{vFile:lstat}
>> +packets, as described in @ref{struct stat64}.
> The "as described in @ref..." paradigm produces nice HTML, but looks
> awkward in Info, because it produces "see".  I suggest a more
> traditional
>
>    ...by the @samp{vFile:fstat}, @samp{vFile:stat} and @samp{vFile:lstat}
>    packets (@pxref{stat64}).

Will fix, here and at the other five places.  My node is named
"struct stat64" (to match the existing "struct stat" node), so I'll use
@pxref{struct stat64} — let me know if you'd prefer the node renamed.

>
>> +If both @value{GDBN} and the stub report the @samp{stat64} feature in
>> +the @samp{qSupported} exchange, the wider format described in
>> +@ref{struct stat64} is used instead.
> Same here.
>
>> +If both @value{GDBN} and the stub report the @samp{stat64} feature in
>> +the @samp{qSupported} exchange, the wider format described in
>> +@ref{struct stat64} is used instead.
> And here.
>
>> +If both @value{GDBN} and the stub report the @samp{stat64} feature in
>> +the @samp{qSupported} exchange, the wider format described in
>> +@ref{struct stat64} is used instead.
> And here.
>
>> +When the @samp{stat64} feature has been negotiated, the buffer returned
>> +by the @samp{vFile:fstat}, @samp{vFile:stat} and @samp{vFile:lstat}
>> +packets uses the following representation instead:
>> +
>> +@smallexample
>> +struct stat64 @{
>> +    unsigned long st_dev;      /* device */
>> +    unsigned long st_ino;      /* inode */
>> +    mode_t        st_mode;     /* protection */
>> +    unsigned long st_nlink;    /* number of hard links */
>> +    unsigned int  st_uid;      /* user ID of owner */
>> +    unsigned int  st_gid;      /* group ID of owner */
>> +    unsigned long st_rdev;     /* device type (if inode device) */
>> +    unsigned long st_size;     /* total size, in bytes */
>> +    unsigned long st_blksize;  /* blocksize for filesystem I/O */
>> +    unsigned long st_blocks;   /* number of blocks allocated */
>> +    unsigned long st_atime;    /* time of last access */
>> +    unsigned long st_mtime;    /* time of last modification */
>> +    unsigned long st_ctime;    /* time of last change */
>> +@};
>> +@end smallexample
> You probably assumed that 'unsigned long' is a 64-bit type.  But that
> is not always true, so I suggest to use a more explicit uint64_t type
> instead.

Good catch - I'd copied the types from the existing struct stat
documentation without thinking about it.  Will use uint64_t.

Thanks for the review.  I'll send a v2 soon.

Abhay

>
>> +This structure is of size 92 bytes.  The fields have the same meanings
>> +as in @ref{struct stat}, but @code{st_dev}, @code{st_ino},
> The "in @ref..." issue again.
>
>> +This representation is only used by the @samp{vFile} packets; the
>> +File-I/O protocol always uses @ref{struct stat}.
> And here.
>
> Thanks.
>
> Reviewed-By: Eli Zaretskii<eliz@gnu.org>

[-- Attachment #2: Type: text/html, Size: 8909 bytes --]