Zephyr types incompatibilities (e.g. u32_t vs uint32_t)

[tgorochowik]

Hello all! I have a question regarding the usage of u32_t with newlib.

So the problem is that as far as I understand all the variables in the drivers should use u32_t, at the same time, the ext files (e.g. HALs) are NOT ported to use these types and still use uint32_t.

What is the correct approach to calling HAL functions in the driver to avoid warnings with different C libs?

Should I simply avoid using u32_t in that cases and cast directly to uint32_t in the driver?

Here is an example of such call: https://github.com/zephyrproject-rtos/zephyr/blob/a313e5c74f01750411bf65e9903278f913328585/drivers/ethernet/eth_sam_gmac.c#L99

The function is defined as follows: https://github.com/zephyrproject-rtos/zephyr/blob/a313e5c74f01750411bf65e9903278f913328585/ext/hal/cmsis/Include/core_cm7.h#L2453

It generates the following warning when newlib is used: ext/hal/cmsis/Include/core_cm7.h:2453:22: note: expected ‘uint32_t * {aka long unsigned int *}’ but argument is of type ‘u32_t * {aka unsigned int *}’

So the typedefs are obviously different.

Thank you for all your insights!

[carlescufi]

Copying @galak since he originally introduced the types.

[SebastianBoe]

Could this simply be a bug in the definition of u32_t ?

It is defined as "unsigned int", but AFAICT that is only guaranteed to be at least 16bit. u32_t should be defined as "unsigned long int" to guarantee at least 32bit.

So this could be resolved by patching zephyr/include/types.h like this:

diff --git a/include/zephyr/types.h b/include/zephyr/types.h
index 5ae80b0fa5..8bb428f2e6 100644
--- a/include/zephyr/types.h
+++ b/include/zephyr/types.h
@@ -20,7 +20,7 @@ typedef signed long long    s64_t;

 typedef unsigned char       u8_t;
 typedef unsigned short      u16_t;
-typedef unsigned int        u32_t;
+typedef unsigned long int   u32_t;
 typedef unsigned long long  u64_t;

 #ifdef __cplusplus

[andrewboie]

u32_t should be defined as "unsigned long int" to guarantee at least 32bit.

A very large reason why it's currently an unsigned int is because people wanted to use %u in printf format code instead of the more cumbersome (but correct) ...%" PRI_u32 "...

edit: %u, not %d

[SebastianBoe]

I see ...

what about

%u

that is not particularly cumbersome, would that work?

EDIT: %u does appear to work.

[tgorochowik]

@SebastianBoe for that change to fix anything, the built-in libc implementation would also have to be changed, I mean this line: https://github.com/zephyrproject-rtos/zephyr/blob/6f61f098037b9b8b88a80d33814ecec26f112d51/lib/libc/minimal/include/stdint.h#L59

Otherwise we would still get these warnings but with the built-in libc instead of newlib.

Maybe I did not explain it good enough but the problem (warning) actually occurs only when using newlib, the problem is that newlib and the built-in libc implementation use different typedefs. As far as I know (I asked about that on IRC some time ago) the u32_t header is actually supposed to somehow work-around these incompatibilities, but I am just not sure how it is supposed to do that.

[SebastianBoe]

Right, I would suggest changing both of them then, assuming there are no unexpected consequences.

Using %u for unsigned formatting seems acceptable to me. %d is for signed integers.

As far as I can read the C standard we may eventually encounter a toolchain that uses 16 bits for "unsigned int" and break uint32_t as that is perfectly valid behaviour. So this change may fix future problems as well.

— maximum value for an object of type unsigned int, UINT_MAX, 2^16 - 1
— maximum value for an object of type unsigned long int, ULONG_MAX, 2^32 − 1

-- http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

I may be misunderstanding something though.

[pfl]

Combining Sam E70 with network sockets that use newlib causes compilation to fail. This causes TLS socket option PR #7118 to fail with:

/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c: In function ‘dcache_invalidate’:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c:104:31: error: passing argument 1 of ‘SCB_InvalidateDCache_by_Addr’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  SCB_InvalidateDCache_by_Addr((u32_t *)start_addr, size_full);
                               ^
In file included from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/atmel/asf/sam/include/same70/same70q21.h:354:0,
                 from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/arch/arm/soc/atmel_sam/same70/soc.h:39,
                 from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c:35:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/cmsis/Include/core_cm7.h:2453:22: note: expected ‘uint32_t * {aka long unsigned int *}’ but argument is of type ‘u32_t * {aka unsigned int *}’
 __STATIC_INLINE void SCB_InvalidateDCache_by_Addr (uint32_t *addr, int32_t dsize)
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c: In function ‘dcache_clean’:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c:117:26: error: passing argument 1 of ‘SCB_CleanDCache_by_Addr’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  SCB_CleanDCache_by_Addr((u32_t *)start_addr, size_full);
                          ^
In file included from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/atmel/asf/sam/include/same70/same70q21.h:354:0,
                 from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/arch/arm/soc/atmel_sam/same70/soc.h:39,
                 from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/ethernet/eth_sam_gmac.c:35:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/cmsis/Include/core_cm7.h:2480:22: note: expected ‘uint32_t * {aka long unsigned int *}’ but argument is of type ‘u32_t * {aka unsigned int *}’
 __STATIC_INLINE void SCB_CleanDCache_by_Addr (uint32_t *addr, int32_t dsize)
                      ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[tgorochowik]

@pfl Yes, that is exactly the same warning I described in my original message.

The only easy way to fix this for both the built-in libc and newlib would be using uint32_t directly in the driver (which I understand is not allowed with the current convention?). That's why I created this issue.

Unless of course the suggestions from @SebastianBoe are implemented, but I am wondering - wouldn't that render the zephyr types (u32_t) completely redundant?

[SebastianBoe]

Unless of course the suggestions from @SebastianBoe are implemented, but I am wondering - wouldn't that render the zephyr types (u32_t) completely redundant?

Not sure what you mean. They are by-design redundant with uint32_t. It's just a different naming convention for the same concept.

E.g. it would be valid for Zephyr to do:

typedef uint32_t u32_t;

AFAICT

[jukkar]

As a quick workaround we could have a hack by checking CONFIG_NEWLIB in eth_sam_gmac.c and then use proper cast there, or should we have a "proper" fix, any opinions?

[nashif]

we need a proper fix, this keeps coming up, I do not think we addressed it fully when we defined our own types last year and it looks like we have a half backed solution right now. Our goal was to switch to newlib by default, but that seems impossible right now with too many failure cases and type conflicts.

[carlescufi]

Discussed at the TSC, for the time being we will explicitly cast to the type expected by the HAL and a new issue will opened to deal with this in a more generic way.

[dleach02]

I've run into this before when interfacing with HAL code. The simple fix is to cast the u32_t to be a uint31_t in the calls to the HAL API. The basic problem is that newlib defines the u32_t as an unsigned int which the C standard states that it is architecture specific what size an unsigned int is so the compiler enhanced warnings will complain as such. The other fix is to define the u32_t as a uint32_t but then you have the problem of ugly PRI macros in the printf. This was hashed through a lot about a year ago.

[andrewboie]

EDIT: %u does appear to work.

It depends on the C library, and whether it uses "long" or not in the definition.

[rakons]

I agree with @SebastianBoe and @dleach02 , creating our own types is just a kind of reinventing the wheel. The inttypes.h and stdint.h files are a part of C99 standard. If we wish to change the naming convention the best way to do it is to use: typedef uint32_t u32_t;

This should remove any warnings without the needs of explicit cast in the code.

BTW: To safely print by %u this variable: u32_t var = 10;

We should use casting here: printf("var = %u", (unsigned int)var);

This is how it should be done by design if you wish not to use PRI_u32 mentioned by @andrewboie . Creating our own variable types to please the user and allow him to use "non safe" way of printing... Agrh... allow me not to continue this comment.

More BTW:

I get an information that you do not wish to relay on newlibc codebase, but this file: https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/stdint.h;h=4abdacb9f2f844159009a21c416838e2f75c7d4d;hb=HEAD seems not to be distributed on GCC license but really free license. According to the header we can copy it into zephyr codebase:

   /*
    * Copyright (c) 2004, 2005 by
    * Ralf Corsepius, Ulm/Germany. All rights reserved.
    *
    * Permission to use, copy, modify, and distribute this software
    * is freely granted, provided that this notice is preserved.
    */ 

[carlescufi]

@galak can you take a look at this thread since the discussion is back on the table?

[galak]

Since Zephyr supports multiple architectures we chose to define {u,s}{8,16,32,64}_t in include/zephyr/types.h consistently across all the arch's. We define the C99 types {u}int{8,16,32,64}_t to match when using the min libc. However when we utilize newlib we end up with a conflict between the Zephyr types and how the C99 types are defined. The change I introduced in PR #8856 changes newlib's definition of the C99 types to match zephyr's view. This removes the issue for newlib.

[ranbochen]

any update?

[dleach02]

I question the medium priority assigned to this. We keep punting this forward with each milestone so it doesn't seem to be a medium priority. Can we move this to low priority?

@nashif @galak

[galak]

I question the medium priority assigned to this. We keep punting this forward with each milestone so it doesn't seem to be a medium priority. Can we move this to low priority?

@nashif @galak

I'm fine w/setting it low. However we should at some point decide if we want to resolve this for the 2.x series so its closed before we get to the LTS 2.x release.

[dleach02]

I'm fine w/setting it low. However we should at some point decide if we want to resolve this for the 2.x series so its closed before we get to the LTS 2.x release.

I personally want this dealt with as it is annoying that the two items are not the same. Maybe we bring this up as a topic starting with the next release (2.2)? In the meantime I'm going to put it to low.

[galak]

We moved to using the C99 types so this should be resolved. If this is still an issue please reopen.