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

["Julio Guerra" <julio@farjump.io>]

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