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

  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.

Tested with https://github.com/farjump/raspberry-pi/blob/ea31c48d7c7eed27d39fb1bec2d3a1d308cb8ae7/sdk/libalpha/include/alpha/fileio.h#L13

2018-04-28  Julio Guerra  <julio@farjump.io>

	* include/gdb/fileio.h: explicit fixed-width integral File I/O protocol types.
	* gdb/common/fileio.c: explicit fixed-width integral File I/O protocol types.
	* gdb/common/fileio.h: explicit fixed-width integral File I/O protocol types.
	* gdb/remote-fileio.c: explicit fixed-width integral File I/O protocol types.

Signed-off-by: Julio Guerra <julio@farjump.io>
---
 gdb/ChangeLog        |  7 ++++++
 gdb/common/fileio.c  | 39 +++++++++++++++--------------
 gdb/common/fileio.h  |  8 +++---
 gdb/remote-fileio.c  | 26 ++++++++++----------
 include/gdb/fileio.h | 58 +++++++++++++++-----------------------------
 5 files changed, 63 insertions(+), 75 deletions(-)
  

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
  

Hello,

Since I am talking a lot of embedded software, I CC'd Joel Brobecker
from AdaCore to maybe give the final direction for a v2.

On 05/02/2018 12:45, Pedro Alves wrote :
> 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.

I was indeed writing a GCC-based solution. Isn't GDB only compiled with GCC?

When compiling embedded programs doing File IOs, I have always been in
bare-metal cases, thus using CFLAGS "-ffreestanding -nostdinc
-nostdlib". So including "stdint.h" would fail because of -nostdinc. And
since my end goal here is to make fileio.h the single source of truth of
File IO protocol types, to then be able to include it in other C/C++
programs, I need it be compiled with these flags, i.e. without the
standard library. And in my opinion, using the File IO protocol in
bare-metal programs is where File IOs shine the most :)

So, if other compilers than GCC are required, I can rewrite it using
standard unsigned int, etc.
> 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)


Yes, and this is why I rather prefer this fileio.h file to become the
single source of truth instead of redefining them and ending with
mistakes such as memory overflows. For example, note the "sizeof fst",
where we are assuming that the target program has correctly redefined
it. Sharing fileio.h with the target will most likely limit the problem.

I indeed missed the alignment. Can't we simply pack the structure
"__attribute__ ((packed))"? I find these implicit "address-of" used
everywhere in GDB's code very opaque, and I am not sure how it would end
up when included in other C/C++ programs. It is anyway a source of
errors since no one expects integral types to be defined as an array, at
least not named like that (for example, I had problems because I wrote
fio_uint* to pass a File IO integer pointer, which ends up as a
"char(*)[32]"...).
> 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.

Sorry, I missed this documentation entry. I'm totally fine in keeping
current type names since if fielio.h ends up installed, we would still
be able to avoid redefiniton mistakes and reuse them.

-- 
Julio Guerra
Co-founder & CTO of Farjump
Mobile: +33 618 644 164
LinkedIn: https://linkedin.com/in/guerrajulio
Slack: farjump.slack.com
  

> Since I am talking a lot of embedded software, I CC'd Joel Brobecker
> from AdaCore to maybe give the final direction for a v2.

I'm not really sure what is expected of me, can you clarify? If you
are thinking of my experience with baremetal targets, that's no problem.
But, although I do work with C code on baremetal targets, it tends
to be in situations where we have a C library (based on newlib, for
the most part).

Are we talking about the remote File-IO extension? I should say up front
that my knowledge of this extension was nearly null until I read the
documentation in the manual. I think Pedro is the expert on that.

If I understand correctly, you are arguing that we should be providing
fileio.h so that programs being debugged can use it? I don't understand
how this would integrate with GDB, though. If I understand correctly,
this file is used for the definition of some structures used within
the remote protocol, so the information exchanged between GDB and
GDBserver. How would you exploit that? (what this tells me is that
your patch series deserves a documentation update)
  

Le 02/05/2018 à 18:14, Joel Brobecker a écrit :
>> Since I am talking a lot of embedded software, I CC'd Joel Brobecker

>> from AdaCore to maybe give the final direction for a v2.

> I'm not really sure what is expected of me, can you clarify? If you

> are thinking of my experience with baremetal targets, that's no problem.

> But, although I do work with C code on baremetal targets, it tends

> to be in situations where we have a C library (based on newlib, for

> the most part).

Yes, this is why I thought of you.
> Are we talking about the remote File-IO extension? I should say up front

> that my knowledge of this extension was nearly null until I read the

> documentation in the manual. I think Pedro is the expert on that.

Yes, we are talking about the File IO extension.
> If I understand correctly, you are arguing that we should be providing

> fileio.h so that programs being debugged can use it? I don't understand

> how this would integrate with GDB, though. If I understand correctly,

> this file is used for the definition of some structures used within

> the remote protocol, so the information exchanged between GDB and

> GDBserver. How would you exploit that? (what this tells me is that

> your patch series deserves a documentation update)

No, these structures in fileio.h are passed from GDB to the target
program being debugged through its GDBserver.

When encapsulated in the newlib, it requires to stub the available
syscalls using this extension (that's what we do) . The situation is the
same at the stub level (redefining File IO structures, changing the
endianness if necessary, and copying them into standard structures (for
struct stat and struct timeval).

Ok, so let's wait Pedro's answer ;)

Thank you,

-- 
Julio Guerra