stdint.h: streamline type definitions

[npitre]

Compilers (at least gcc and clang) already provide definitions to
create standard types and their range. For example, INT16_TYPE is
normally defined as a short to be used with the int16_t typedef, and
INT16_MAX is defined as 32767. So it makes sense to rely on them
rather than hardcoding our own, especially for the fast types where
the compiler itself knows what basic type is best.

Using compiler provided definitions makes even more sense when dealing
with 64-bit targets where some types such as intptr_t and size_t must
have a different size and range. Those definitions are then adjusted
by the compiler directly.

However there are two cases for which we should override those
definitions:

• The INT32_TYPE definition on 32-bit targets vary between an int
  and a long int depending on the architecture and configuration.
  Notably, all compilers shipped with the Zephyr SDK, except for the
  i586-zephyr-elfiamcu variant, define INT32_TYPE to a long int.
  Whereas, all Linux configurations for gcc, both 32-bit and 64-bit,
  always define INT32_TYPE as an int. Having variability here is
  not welcome as pointers to a long int and to an int are not deemed
  compatible by the compiler, and printing an int32_t defined with a
  long using %d makes the compiler to complain, even if they're the
  same size on 32-bit targets. Given that an int is always 32 bits
  on all targets we might care about, and given that Zephyr hardcoded
  int32_t to an int before, then we just redefine INT32_TYPE and
  derrivatives to an int to keep the peace in the code.

• The confusion also exists with INTPTR_TYPE. Looking again at the
  Zephyr SDK, it is defined as an int, even even when INT32_TYPE is
  initially a long int. One notable exception is i586-zephyr-elf where
  INTPTR_TYPE is a long int even when using -m32. On 64-bit targets
  this is always a long int. So let's redefine INTPTR_TYPE to always
  be a long int on Zephyr which simplifies the code, works for both
  32-bit and 64-bit targets, and mimics what the Linux kernel does.
  Only a few print format strings needed adjustment.

In those two cases, there is a safeguard to ensure the type we're
enforcing has the right size and fail the build otherwise.

[andyross]

Heh, as soon as I saw this PR I went to see if it was failing on newlib, and of course it is. :)

I approve in principle, but note that the newlib "support" in gcc is an infuriating mess. Lots of stuff you expect would be invariant with the toolchain turns out to be sensitive to the C library, and in particular newlib wants the int32_t type to be "long" and not "int" on 32 bit platforms, even though you'd think that wouldn't make any difference. Or something. I forget the details but remember them being painful.

[npitre]

On Wed, 5 Jun 2019, Andy Ross wrote: I approve in principle, but note that the newlib "support" in gcc is an infuriating mess. Lots of stuff you expect would be invariant with the toolchain turns out to be sensitive to the C library, and in particular newlib wants the int32_t type to be "long" and not "int" on 32 bit platforms, even though you'd think that wouldn't make any difference. Or something. I forget the details but remember them being painful.

Zephyr already hardcoded int32_t to an int so nothing really changed here. From a quick look in the newlib source tree, the file newlib/libc/include/machine/_default_types.h appears to define __int32_t using __INT32_TYPE__ if that exists, or using signed int if __INT_MAX__ gives the right answer, or signed long if __LONG_MAX__ gives the right answer, or even signed short if __SHRT_MAX__ gives the right answer, or finally using signed char if __SCHAR_MAX__ gives the right answer. Failing all that and it is undefined. So in principle newlib should fall back to signed int if __INT32_TYPE__ is not defined, but as I've just discovered, gcc appears to define __INT32_TYPE__ somewhat randomly between an int and a long. I really think that Zephyr should promote more sanity and enforce some logic and consistency here, just like Linux does.

[andrewboie]

In principle I have nothing against this patch and it seems like a good change...but need to resolve newlib issues

[andrewboie]

this change is breaking builds with newlib enabled

[npitre]

On Thu, 6 Jun 2019, Andrew Boie wrote: this change is breaking builds with newlib enabled

Any primer on how to do that somewhere?

[andrewboie]

click on the details for the failed shippable run, you will see logs for failing test cases

[npitre]

On Thu, 6 Jun 2019, Andrew Boie wrote: click on the details for the failed shippable run, you will see logs for failing test cases

Well... that's an issue for me. I'm visually handicaped and anything that requires a javascript enabled browser is hit and miss for my accessibility tools. In this case this is a miss. Is there any other way to access this information?

[andrewboie]

If you run sanitycheck locally you should see the failures.

The tests failing are:

tests/net/socket/getnameinfo/net.socket
tests/net/socket/tcp/net.socket.tcp
tests/lib/mem_alloc/libraries.libc.newlib
tests/net/socket/misc/net.socket
tests/net/socket/select/net.socket

on mps2_an385 target.

[npitre]

Those tests all fail for me whether or not my patch is applied.

[andrewboie]

Builds just fine for me.

Example.

cd tests/lib/mem_alloc
mkdir out
cd out
cmake -DCONF_FILE=prj_newlib.conf -DBOARD=mps2_an385 ..
make -j8

[npitre]

That builds and runs just fine for me too, with or without my patch. What gives?

[andyross]

Toolchain version maybe? Again, the root of the "integer types vs. newlib" problem is that the gcc toolchain headers will end up detecting newlib very early and exposing different types via what you would think are gcc-only preprocessor symbols. Maybe this finally got addressed upstream?

[npitre]

On Thu, 6 Jun 2019, Andy Ross wrote: Toolchain version maybe?

I'm using the latest Zephyr SDK. I suppose the CI infrastructure does too, right?

Again, the root of the "integer types vs. newlib" problem is that the gcc toolchain headers will end up detecting newlib very early and exposing different types via what you would think are gcc-only preprocessor symbols. Maybe this finally got addressed upstream?

I somehow doubt it.

[npitre]

OK, I managed to reproduce a test failure here. I'm on it.

[SebastianBoe]

What do you think about using the gcc option

-imacros enforce_standard_type_correspondance.h

instead here?

This would shorten the command line and allow us to use

#undef

and

#define

which readers will be more familiar with.

Also allows us to avoid using SHELL, which I am not familiar with, and add_compile_options, which causes problems for the zephyr_get_* API.

imacro should generally be avoided, as symbols should not magically appear in the source, but symbols that are prefixed with __ shouldn't be expected to be in-source anyway.

[npitre]

On Wed, 12 Jun 2019, Sebastian Bøe wrote: What do you think about using the gcc option -imacros enforce_standard_type_correspondance.h instead here?

I think this is an excellent idea. And not only for the newlib case but for all cases. However I'm currently fighting cmake who thinks it knows better. It somehow interprets "-imacros" as an include file directive and insists on deduplicating them. There is already `-imacros ${AUTOCONF_H}` in the argument list, so whichever specified second one gets its `-imacros` stripped away.

[npitre]

On Wed, 12 Jun 2019, Sebastian Bøe wrote: Also allows us to avoid using SHELL, which I am not familiar with, and add_compile_options, which causes problems for the zephyr_get_* API.

Looks like there is no way around that. the `-imacros` argument becomes used twice, and CMake will deduplicate the second one. The CMake manual expressly documents the workaround for avoiding argument deduplication and that implies the SHELL: construct.

[SebastianBoe]

Was not aware --flag=value was supported for imacros. This simplifies things.

Nice find.

[SebastianBoe]

Sorry, but now that this commit: "4ddbc0096a5e9925a27ee3a0b130fd78373650b6"

is merged, we must, for toolchain portability reasons, use

toolchain_cc_imacros

instead of

zephyr_compile_options

and patch this line:

4ddbc00#diff-6c754805e58aa2a9bce0dbdf0414d7f5R7

to use the format that does not have whitespace, meaning "--imacros=foo" instead of "-imacros foo".

[andrewboie]

@SebastianBoe can you re-visit your review?

[ghost]

I am uncomfortable with this patch because it relies on compiler-specific features on two levels: first, it relies on the existence of INT16_TYPE et al., and second, it relies on a non-standard feature of the preprocessor (-imacros). This seems to be at odds with the works that's been underway for quite a while to make the build process and the codebase itself toolchain-independent.

Is there a problem we're trying to solve? Is the medicine worse than the cure? Am I misunderstanding something?

[npitre]

On Mon, 24 Jun 2019, Charles E. Youse wrote: I am uncomfortable with this patch because it relies on compiler-specific features on two levels: ...

Most libc flavors already rely on those to define their own basic types. Both gcc and clang do provide them. If some odd compiler doesn't provide those symbols then it is pretty easy to simply #define them in a dedicated header file for that compiler. And -imacros may be replaced by -include just as well (it only needs to be listed first and contain nothing else than #define's). This isn't the first use of imacros either.

Is there a problem we're trying to solve?

Absolutely. We already decided that using long over intptr_t is best. However some gcc configurations define intptr_t as an int, some as a long. And some gcc configurations define int32_t as a long, others as an int. In short, it is a complete mess. This is troublesome on multiple levels, especially when using printf (sometimes it is %x, sometimes %lx) depending on the target you compile for. Sometimes casting those types to a pointer will work sometimes not. And this trickes down into newlib as well. Here we're setting the standard for the entire Zephyr codebase. We let the compiler provide its idea of what is the best basic type, and override it when we know better. With this, int32_t is _always_ an int, intptr_t is _always_ a long, and printf format conversion specifiers don't have to change whether we're building for 32 or 64 bits, etc. That will save our sanity in the long run.

[ghost]

And -imacros may be replaced by -include just as well (it only needs to be listed first and contain nothing else than #define's). This isn't the first use of imacros either.

-include is just as build-system specific as -imacros. Neither is a standard feature. But, fair enough, these aren't new occurrences so they'll both have to be dealt with anyway.

That will save our sanity in the long run.

Oh, good. I have such little sanity left. :-)

[andrewboie]

@SebastianBoe ping

[pfalcon]

@npitre: Thanks for tackling this issue. There's a bit of heritage in Zephyr with this stuff. The matter, either you embrace all the goodies the contemporary C standards bring. And besides nice things like uint32_t and uintptr_t, that also includes more "cumbersome" things like "foo" PRIi8 "bar". If you don't embrace all those things, the only way is to reinvent your own wheel, and that's how Zephyr got its own types like u8_t (yeah, I guess the motivation also was "if Linux does it, why can't we").

Unfortunately, that reinventing is not one-time thing, it keeps biting going forward, disallowing to use nice things like intptr_t. And it doesn't stop at that level either, because we have further conflicts on the POSIX subsys vs Newlib, etc. level. Bottom line: I have to admit I'm a bit concerned to hear about newlib issues mentioned with this patch, but I think the only way to deal with all those issues is to resolve them, step by step, instead of avoid using workarounds, more and more. In that regard, this patch looks like going in the right direction, and if it passes CI, +1 should be just the right choice for it. Thanks.