Re: [PATCH 3/4] Explicit fixed-width integral File I/O protocol types

[Pedro Alves <palves@redhat.com>]

On 04/28/2018 02:19 AM, Julio Guerra wrote:
> The File I/O extension defines portable types of there host-specific
> counterparts, such as `struct stat` and `struct timeval`. This patch improves
> these type definitions in `include/gdb/fileio.h` to make possible sharing them
> with target programs, and avoid redefining them by being able to include this
> header, even with cross-compiled programs.
> 
> The patch thus removes several drawbacks:
> - avoid implicit pointers when defining fixed-width integers as array typedefs.
> - explicitly state the sizes of fixed-width integers (e.g. fio_ulong_t becomes
>   fio_uint64_t).
> It also renames a few misnamed conversion functions with the convention
> `host_to_fileio_*` used everywhere else.
> 
> Note that fixed-width integer types are defined using GCC's preprocessor builtin
> macros to avoid using the libc's stdint.h which might not be available on the
> target compiler. Therefore, `include/gdb/fileio.h` is standalone.

Sorry, but NAK.

I don't see how using GCC preprocessor builtins would be more
portable than stdint.h.  That doesn't make the file
standalone -- it makes it dependent on gcc instead.

But regardless, the major problem here is that this approach
does not work, because it ignores the issue of alignment and
padding holes.
With the current approach, all structure fields are aligned
to 1 byte.  With the explicit types approach, fields are
aligned to their natural alignment.  E.g., from a glance,
it seems like we'd be creating a 4-byte padding hole after
fst_rdev.  Even if by chance the fields end up aligned,
that's not something we should be relying on.  Both host
and client must agree on the layout of the data structures.
fio_stat objects are copied directly into target memory,
see tail end of remote-fileio.c:remote_fileio_func_stat,
for example:

  if (statptr)
    {
      host_to_fileio_stat (&st, &fst);
      host_to_fileio_uint (0, fst.fst_dev);

      errno = target_write_memory (statptr, (gdb_byte *) &fst, sizeof fst);
      if (errno != 0)

As for the "int"/"long"/etc naming vs explicitly-sized uint32_t etc.,
note that the File I/O protocol's "int" "long" etc. types are defined here:

 https://sourceware.org/gdb/current/onlinedocs/gdb/Integral-Datatypes.html#Integral-Datatypes

I don't have a strong opinion, but I'm not sure it's worth it to
deviate from the documentation.

Thanks,
Pedro Alves