Re: [PATCH v3 2/2] gdb: add stat64 and fstat64 to the F-packet file-i/o protocol

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

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

Hi Ulrich,

Thanks for looking.

On 27/10/26 23:17, Ulrich Weigand wrote:
> Abhay Kandpal<abhay@linux.ibm.com> wrote:
>
>> The F-packet calls cannot use qSupported, since the buffer is read
>> directly by the target.  But each request names the call it wants, so
>> add two new calls, Fstat64 and Ffstat64, which are identical to Fstat
>> and Ffstat except that they write struct fio_stat_wide.  Existing
>> targets keep using Fstat and Ffstat and see no change.  A GDB which
>> does not know the new calls already replies with ENOSYS, so a target
>> can try Fstat64 and fall back to Fstat.
> This makes sense to me, if support for this is needed.  I'm not
> really sure who -if anyone- is using these F-packets; gdbserver
> doesn't seem to do so and there's no test in the test suite ...
>
> Andrew, do you have any thoughts on this?

One user is QEMU: its semihosting code (semihosting/syscalls.c)
sends F-packet requests for every call in the protocol, including
both stat and fstat. I added 2/2 because Andrew preferred both
directions to be fixed, and I'm happy to go either way.

>
>
>> The console case in the fstat handler stored 1 into fst_dev, but that
>> was then overwritten when the struct was packed, so the target always
>> saw zero.  Drop the dead store; behaviour is unchanged.
> Interesting.  Seems this accidentally changed here:
> https://sourceware.org/pipermail/gdb-patches/2015-March/122143.html
>
> which says:
> 	(remote_fileio_mode_to_target): Moved to common-remote-
> fileio.c.
> 	(remote_fileio_to_fio_mode): Likewise.
> 	(remote_fileio_to_fio_ulong): Likewise.
> 	(remote_fileio_to_fio_stat): Likewise.
>
> but in addition to the move, changed:
>
> -static void
> -remote_fileio_to_fio_stat (struct stat *st, struct fio_stat *fst)
> -{
> -  LONGEST blksize;
> -
> -  /* `st_dev' is set in the calling function.  */
> -  remote_fileio_to_fio_uint ((long) st->st_ino, fst->fst_ino);
>
> to:
>
> +void
> +remote_fileio_to_fio_stat (struct stat *st, struct fio_stat *fst)
> +{
> +  LONGEST blksize;
> +
> +  remote_fileio_to_fio_uint ((long) st->st_dev, fst->fst_dev);
> +  remote_fileio_to_fio_uint ((long) st->st_ino, fst->fst_ino);
>
>
> So up until 2015 fst_dev was indeed 1 for the console and
> 0 for everything else, and since then it's been 0 always.
> Given that long time, it may be fine to keep it at 0 now ...
>
> On the other hand, the documentation still says:
>
> @item st_dev
> A value of 0 represents a file, 1 the console.
>
> So we can either fix the code again, or else update the
> documentation to match the behavior over the last 10 years.
> Thoughts?

There is a second mismatch as well. For a regular file, the fstat
handler never sets fst_dev, so the host's real st_dev is passed
through rather than the documented 0. Today only stat matches
the documentation:

stat, file:        documented 0, sends 0
fstat, console:    documented 1, sends 0
fstat, file:       documented 0, sends the host's st_dev

My preference would be to fix the code to match the documentation,
since the 2015 change looks accidental. I'd do that as a separate
patch, so it doesn't depend on the stat64 series.

Thanks
Abhay

>
> Bye,
> Ulrich

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