Re: [PATCH v3 1/2] gdbserver: widen the stat fields returned by the vFile packets

[Andrew Burgess <aburgess@redhat.com>]

Abhay Kandpal <abhay@linux.ibm.com> writes:

> The struct sent by the vFile:fstat, vFile:stat and vFile:lstat packets
> declares st_dev, st_ino, st_nlink, st_rdev and the three timestamps as
> 4-byte fields, while the corresponding struct stat members are 8 bytes
> on a 64-bit host.  The values are silently truncated.
>
> This is not visible on a filesystem whose inode numbers stay below
> 2^32, but it is on a large one.  On an 86 TB xfs volume,
> gdb.server/fileio-packets.exp fails because the inode reported by
> vFile:lstat does not match the one reported locally:
>
>   remote = {..., 'st_ino': 2161233200, ...}
>   local  = {..., 'st_ino': 122420317488, ...}
>
> The field widths cannot simply be changed, as that would alter the wire
> layout and break communication between different versions of GDB and
> gdbserver.  Add a new 'stat64' qSupported feature instead.  When both
> sides report it, the vFile stat packets return struct fio_stat_wide, in
> which the six too-narrow fields are 8 bytes; otherwise the existing

Isn't is seven fields?  st_dev, st_ino, st_nlink, st_rdev and the three
timestamps.

> struct fio_stat is used, exactly as before.
>
> Internally both GDB and gdbserver now work with the wide struct, and
> the conversion happens at the wire boundary: gdbserver narrows the
> result before sending it if the feature was not negotiated, and GDB
> selects the unpacker by the length of the reply.  Narrowing a value
> which does not fit now produces a warning rather than truncating
> silently.
>
> The F-packet file-i/o protocol uses the same struct, but writes it
> into inferior memory rather than sending it as packet data, so it
> cannot use qSupported negotiation.  It is handled separately in the
> next patch.
>
> While here, drop a duplicate include of gdbsupport/fileio.h from
> gdbserver/hostio.cc.
>
> Tested on powerpc64le-linux; gdb.server/fileio-packets.exp now passes on
> a filesystem with inode numbers above 2^32 and continues to pass on one
> without, with no unexpected failures in the full testsuite.
>
> 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.

These style of changelog entries are no longer used in GDB and shouldn't
be included.



> @@ -13656,29 +13667,35 @@ remote_target::fileio_readlink (struct inferior *inf, const char *filename,
>     remote syscall reply.  EXPECTED_LEN is the length returned from the
>     fstat or stat call, this the length of the returned data (in ATTACHMENT)
>     once it has been decoded.  The fstat/stat result (from the ATTACHMENT
> -   data) is to be placed in ST.  */
> +   data) is to be placed in ST.
>  
> +   The reply holds either a struct fio_stat or, if the stat64 feature was
> +   negotiated, the wider struct fio_stat_wide; the two are distinguished
> +   by their length.  */
>  static int

I think you lost the blank line between the comment and the 'static int' line.

>  fileio_process_fstat_and_lstat_reply (const char *attachment,
>  				      int attachment_len,
>  				      int expected_len,
>  				      struct stat *st)
>  {
> -  struct fio_stat fst;
> +  gdb_byte buf[sizeof (struct fio_stat_wide)];
>  
>    int read_len
>      = remote_unescape_input ((gdb_byte *) attachment, attachment_len,
> -			     (gdb_byte *) &fst, sizeof (fst));
> +			     buf, sizeof (buf));
>  


> @@ -519,16 +533,31 @@ handle_stat (char *own_buf, int *new_packet_len)
>        return;
>      }
>  
> -  host_to_fileio_stat (&st, &fst);
> -
> -  bytes_sent = hostio_reply_with_data (own_buf,
> -				       (char *) &fst, sizeof (fst),
> -				       new_packet_len);
> +  struct fio_stat_wide wide;
> +  host_to_fileio_stat_wide (&st, &wide);
>  
> -  /* If the response does not fit into a single packet, do not attempt
> -     to return a partial response, but simply fail.  */
> -  if (bytes_sent < sizeof (fst))
> -    write_enn (own_buf);
> +  /* Send the wide form if GDB understands it, otherwise narrow the
> +     result first.  */
> +  if (cs.stat64_feature)
> +    {
> +      bytes_sent = hostio_reply_with_data (own_buf, (char *) &wide,
> +					   sizeof (wide), new_packet_len);
> +      /* If the response does not fit into a single packet, do not attempt
> +	 to return a partial response, but simply fail.  */
> +      if (bytes_sent < sizeof (wide))
> +	write_enn (own_buf);
> +    }
> +  else
> +    {
> +      struct fio_stat narrow;
> +      fileio_stat_wide_to_narrow (&wide, &narrow);
> +      bytes_sent = hostio_reply_with_data (own_buf, (char *) &narrow,
> +					   sizeof (narrow), new_packet_len);
> +      /* If the response does not fit into a single packet, do not attempt
> +	 to return a partial response, but simply fail.  */
> +      if (bytes_sent < sizeof (narrow))
> +	write_enn (own_buf);
> +    }

This pattern is repeated 3 times.  I wonder if you could factor out a
helper function, hostio_send_stat_reply,  to handle this duplication?

>  }
>  
>  static void


> diff --git a/gdbserver/server.h b/gdbserver/server.h
> index bbba3650de7..cf811f228fa 100644
> --- a/gdbserver/server.h
> +++ b/gdbserver/server.h
> @@ -192,6 +192,11 @@ struct client_state
>    /* If true, memory tagging features are supported.  */
>    bool memory_tagging_feature = false;
>  
> +  /* True if GDB supports the wide form of struct stat in the vFile
> +     fstat, stat and lstat replies.  When false, gdbserver narrows the
> +     result to struct fio_stat before sending it.  */
> +  bool stat64_feature = false;

If you search in gdbserver/server.cc for the comment text 'Be sure we're
out of tfind mode', you'll find a block of code that resets all of these
feature flags.

There is already a bug (introduced by me) for a feature flag that is not
being reset in this block correctly.  But your flag also needs resetting
here too.  Without this, then when we reconnect the feature flag from
the previous connection will remain turned on.

> +
>    /* If true then E.errtext style errors are supported everywhere,
>       including for the qRcmd and m packet.  When false E.errtext errors
>       are not supported with qRcmd and m packets, but are still supported


> @@ -199,4 +238,13 @@ host_to_fileio_time (time_t num, fio_time_t fnum)
>  
>  extern void host_to_fileio_stat (struct stat *st, struct fio_stat *fst);
>  
> +/* Pack a host-format struct stat into a struct fio_stat_wide.  */
> +extern void host_to_fileio_stat_wide (struct stat *st,
> +				      struct fio_stat_wide *fst);
> +
> +/* Narrow a struct fio_stat_wide into a struct fio_stat, warning if any
> +   value does not fit.  */
> +extern void fileio_stat_wide_to_narrow (const struct fio_stat_wide *wide,
> +					struct fio_stat *narrow);

GDB style leaves a blank line after the comment and before a function
declaration or definition.  Mostly.  We're not perfect.  But that's what
it's supposed to be.  This file looks to be following the style, so it
would be nice if this new code did too please.

There's also a place in gdbserver/hostio.h which should be fixed too
please.

Thanks,
Andrew

> +
>  #endif /* GDBSUPPORT_FILEIO_H */
> -- 
> 2.52.0