It looks like newlib and our mini libc define uint32_t differently and this causes issues with the printf format warning from gcc. We get things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned int uint32_t;

Mini libc:

typedef unsigned int uint32_t;

So wondering if we should change mini-libc to match and fix up all the build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd party pre-built toolchains

- k

On 19 Jan 2017, at 14:41, Kumar Gala <kumar.gala(a)linaro.org> wrote:

It looks like newlib and our mini libc define uint32_t differently and this causes issues with the printf format warning from gcc. We get things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned intuint32_t;

Mini libc:

typedef unsigned intuint32_t;

So wondering if we should change mini-libc to match and fix up all the build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd party pre-built toolchains

- k

Hi, This is because the code is using the wrong format specifier, see JIRA-1181

Cheers
/Marcus
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Hi,

[Second attempt at responding, my arm email address seems to have
triggered a moderator approval, sorry about the duplication for those
of you that got a direct reply.]

This is because the code is using the wrong format specifier, see
JIRA-1181 https://jira.zephyrproject.org/browse/ZEP-1181

Cheers
/Marcus

Hi Kumar,

On Thu, Jan 19, 2017, Kumar Gala wrote:

It looks like newlib and our mini libc define uint32_t differently and
this causes issues with the printf format warning from gcc. We get
things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned int uint32_t;

Mini libc:

typedef unsigned int uint32_t;

So wondering if we should change mini-libc to match and fix up all the
build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd
party pre-built toolchains

The right format specifier for unsigned integers is %u and not %d, so as
far as I see that's the issue and using %u should make the error go
away.

Johan

On 19 January 2017 at 14:52, Johan Hedberg <johan.hedberg(a)intel.com> wrote:

Hi Kumar,

On Thu, Jan 19, 2017, Kumar Gala wrote:

It looks like newlib and our mini libc define uint32_t differently and
this causes issues with the printf format warning from gcc. We get
things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned int uint32_t;

Mini libc:

typedef unsigned int uint32_t;

So wondering if we should change mini-libc to match and fix up all the
build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd
party pre-built toolchains

The right format specifier for unsigned integers is %u and not %d, so as
far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct specifier is:
PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Cheers
/Marcus

On Jan 19, 2017, at 8:52 AM, Johan Hedberg <johan.hedberg(a)intel.com> wrote:

Hi Kumar,

On Thu, Jan 19, 2017, Kumar Gala wrote:

It looks like newlib and our mini libc define uint32_t differently and
this causes issues with the printf format warning from gcc. We get
things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned int uint32_t;

Mini libc:

typedef unsigned int uint32_t;

So wondering if we should change mini-libc to match and fix up all the
build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd
party pre-built toolchains

The right format specifier for unsigned integers is %u and not %d, so as
far as I see that's the issue and using %u should make the error go
away.

Johan

Thought that myself, but with %u you get:

In file included from /home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:23:0:
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]
BT_ERR("Required RAM size: %u, supplied: %u.", err,


- k

Hi Marcus,

On Thu, Jan 19, 2017, Marcus Shawcroft wrote:

The right format specifier for unsigned integers is %u and not %d, so as
far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct specifier is:
PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Does "correct" in this case have any practical significance? It worsens
the readability of the code quite a lot IMO, so if the significance is
purely theoretical it's a quite high price to pay for correctness. I've
used the PRI* macros in other projects, but never for anything smaller
than 64 bit integers.

Johan

On Jan 19, 2017, at 8:57 AM, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote:

On 19 January 2017 at 14:52, Johan Hedberg <johan.hedberg(a)intel.com> wrote:

Hi Kumar,

On Thu, Jan 19, 2017, Kumar Gala wrote:

It looks like newlib and our mini libc define uint32_t differently and
this causes issues with the printf format warning from gcc. We get
things like:

/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:387:10: error: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka long unsigned int}' [-Werror=format=]

As newlib

typedef long unsigned int uint32_t;

Mini libc:

typedef unsigned int uint32_t;

So wondering if we should change mini-libc to match and fix up all the
build issues associated with this?

Other ideas? Concern that fixing newlib will have issues w/other 3rd
party pre-built toolchains

The right format specifier for unsigned integers is %u and not %d, so as
far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct specifier is:
PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Cheers
/Marcus

Not having much luck there, but its possible I’m missing something obvious because of lack of sleep:

In file included from /home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:24:0:
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:388:33: error: expected ')' before 'PRIu32'
BT_ERR("Required RAM size: %" PRIu32 ", supplied: %u.", err,
^

Here’s what BT_ERR looks like:
BT_ERR("Required RAM size: %" PRIu32 ", supplied: %u.", err,
sizeof(_radio));


- k

#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Cheers
/Marcus

Not having much luck there, but its possible I’m missing something obvious because of lack of sleep:

In file included from /home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:24:0:
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:388:33: error: expected ')' before 'PRIu32'
BT_ERR("Required RAM size: %" PRIu32 ", supplied: %u.", err,
^

That would be my fault, my example above is missing:

#include <inttypes.h>

Cheers
/Marcus

On Jan 19, 2017, at 9:15 AM, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote:

#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Cheers
/Marcus

Not having much luck there, but its possible I’m missing something obvious because of lack of sleep:

In file included from /home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:24:0:
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c: In function 'hci_driver_open':
/home/galak/git/zephyr-project/subsys/bluetooth/controller/hci/hci_driver.c:388:33: error: expected ')' before 'PRIu32'
BT_ERR("Required RAM size: %" PRIu32 ", supplied: %u.", err,
^

That would be my fault, my example above is missing:

#include <inttypes.h>

Cheers
/Marcus

Ah, that fixes things.

Now to the question about how uint32_t should be defined ;)

- k

On Thu, Jan 19, 2017 at 10:03 AM, Johan Hedberg <johan.hedberg(a)intel.com>
wrote:

Hi Marcus,

On Thu, Jan 19, 2017, Marcus Shawcroft wrote:

The right format specifier for unsigned integers is %u and not %d, so

as

far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct

specifier is:

PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Does "correct" in this case have any practical significance? It worsens
the readability of the code quite a lot IMO, so if the significance is
purely theoretical it's a quite high price to pay for correctness. I've
used the PRI* macros in other projects, but never for anything smaller
than 64 bit integers.

We need to take into consideration 3rd party code that we can't modify, so
while it might work for the kernel code, we will still have issues with 3rd
party code using %u.


Anas

Johan

On 19 January 2017 at 15:39, Anas Nashif <nashif(a)gmail.com> wrote:

On Thu, Jan 19, 2017 at 10:03 AM, Johan Hedberg <johan.hedberg(a)intel.com>
wrote:

Hi Marcus,

On Thu, Jan 19, 2017, Marcus Shawcroft wrote:

The right format specifier for unsigned integers is %u and not %d, so
as
far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct
specifier is:
PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Does "correct" in this case have any practical significance? It worsens
the readability of the code quite a lot IMO, so if the significance is
purely theoretical it's a quite high price to pay for correctness. I've
used the PRI* macros in other projects, but never for anything smaller
than 64 bit integers.

We need to take into consideration 3rd party code that we can't modify, so
while it might work for the kernel code, we will still have issues with 3rd
party code using %u.

We will likely find either immediately, or with future additions to
/ext that each blob of third party code does something different:
- Some will use %u for uint32_t
- Some will use %lu for uint32_t
- Some might even actually use the inttypes.h defs.
- Some will have hacked the problem already with stuff like "%lu",
(long int) some_uint32_variable
...

Clearly we can't appease the first two simultaneously by futzing with
newlib, gcc etc

If we want to get diagnostic clean from the compiler we may end up
needing to explicitly disable -Wformat on /ext

Cheers
/Marcus

Hi Anas,

On Thu, Jan 19, 2017, Anas Nashif wrote:

On Thu, Jan 19, 2017, Marcus Shawcroft wrote:

The right format specifier for unsigned integers is %u and not %d, so

as

far as I see that's the issue and using %u should make the error go

Since the type is 'uint32_t' rather than 'unsigned' the correct

specifier is:

PRIu32

e.g.
#include <stdint.h>
printf("blah %" PRIu32 " more blah", v);

Does "correct" in this case have any practical significance? It worsens
the readability of the code quite a lot IMO, so if the significance is
purely theoretical it's a quite high price to pay for correctness. I've
used the PRI* macros in other projects, but never for anything smaller
than 64 bit integers.

We need to take into consideration 3rd party code that we can't modify, so
while it might work for the kernel code, we will still have issues with 3rd
party code using %u.

I'd just like to add to this that while working with Linux and BlueZ
we've used %u extensively for uint8_t, uint16_t and uint32_t without any
issues (I'd think those projects have been run on a larger set of
architectures and C libraries than what Zephyr supports). The only case
where we've had resort to PRI* is uint64_t, and if you check the actual
definition of these in your /usr/include/inttypes.h you'll see that it's
the only one that evaluates to something else than just "u".

Generally I'm a big fan of doing things correctly & properly, but this
stuff just uglifies the format strings too much IMO :)

Johan

Hi!

Generally I'm a big fan of doing things correctly & properly, but this
stuff just uglifies the format strings too much IMO :)

Unless you don't want to exclude the possibility to run Zephyr on 8-bit and
16-bit architectures.

Cheers,
Oleg

That would be my fault, my example above is missing:

#include <inttypes.h>

Cheers
/Marcus

Ah, that fixes things.

Now to the question about how uint32_t should be defined ;)

- k

I'm not seeing a problem here. When you print uint32_t, you use PRIu32. This goes for all the fixed-size types, its why inttypes.h exists.
Newlib and minimal libc both have their own inttypes.h which are defined properly for that C library.

Andrew

On Jan 19, 2017, at 11:35 AM, Boie, Andrew P <andrew.p.boie(a)intel.com> wrote:

That would be my fault, my example above is missing:

#include <inttypes.h>

Cheers
/Marcus

Ah, that fixes things.

Now to the question about how uint32_t should be defined ;)

- k

I'm not seeing a problem here. When you print uint32_t, you use PRIu32. This goes for all the fixed-size types, its why inttypes.h exists.
Newlib and minimal libc both have their own inttypes.h which are defined properly for that C library.

Andrew

As Anas said, I don’t think we can expect 3rd party software to utilize PRIu32. The fact that newlib and minimal libc different in the way the typedef (u)int32_t seems like a pointless difference for us to maintain and have to deal with a lot of pain if one switches between them.

For example, trying building by setting CONFIG_NEWLIB_LIBC=y in tests/include/test.config and see how much breaks.

- k

On 20 January 2017 at 04:06, Kumar Gala <kumar.gala(a)linaro.org> wrote:

As Anas said, I don’t think we can expect 3rd party software to utilize PRIu32. The fact that newlib and minimal libc different in the way the typedef (u)int32_t seems like a pointless difference for us to maintain and have to deal with a lot of pain if one switches between them.

This is not a choice between alternatives. There are two separate
decisions here:

1) Do we align minimal C with newlib in order to have consistent
breakage in /ext irrespective of the users choice between newlib and
minimal lib.

2) Do we write portable conforming code in our own tree outside of
/ext using the standards defined PRI macros.


We can do both, they are not alternatives:
- Number 2 is valuable because writing portable code leaves our
options open in the future.
- Number 1 is valuable because it will ensure consistent breakage under /ext.

Ironically, that said, not doing 1) helps with 2) because it flushes
out broken code.

But either way, whether or not we do 1) I strongly advocate that for
our own code we adopt 2).

Cheers
/Marcus

Hi Marcus,

On 20 January 2017 at 04:06, Kumar Gala <kumar.gala(a)linaro.org> wrote:

As Anas said, I don’t think we can expect 3rd party software to utilize PRIu32. The fact that newlib and minimal libc different in the way the typedef (u)int32_t seems like a pointless difference for us to maintain and have to deal with a lot of pain if one switches between them.

This is not a choice between alternatives. There are two separate
decisions here:

1) Do we align minimal C with newlib in order to have consistent
breakage in /ext irrespective of the users choice between newlib and
minimal lib.

2) Do we write portable conforming code in our own tree outside of
/ext using the standards defined PRI macros.


We can do both, they are not alternatives:
- Number 2 is valuable because writing portable code leaves our
options open in the future.
- Number 1 is valuable because it will ensure consistent breakage under /ext.

Ironically, that said, not doing 1) helps with 2) because it flushes
out broken code.

But either way, whether or not we do 1) I strongly advocate that for
our own code we adopt 2).

I'd rather stick with 1). User PRI for all uint32_t is going to be ugly.
I align with Johan here.

newlib has a reason to use long unsigned int as uint32_t. It ensures
that uint32_t is always 32 bits whatever the
arch newlib is ported on. unsigned int on 16bits arch, is 16 bits arch
for instance.

Tomasz

On Fri, Jan 20, 2017 at 5:26 AM, Marcus Shawcroft <
marcus.shawcroft(a)gmail.com> wrote:

On 20 January 2017 at 04:06, Kumar Gala <kumar.gala(a)linaro.org> wrote:

As Anas said, I don’t think we can expect 3rd party software to utilize

PRIu32. The fact that newlib and minimal libc different in the way the
typedef (u)int32_t seems like a pointless difference for us to maintain and
have to deal with a lot of pain if one switches between them.

This is not a choice between alternatives. There are two separate
decisions here:

1) Do we align minimal C with newlib in order to have consistent
breakage in /ext irrespective of the users choice between newlib and
minimal lib.

2) Do we write portable conforming code in our own tree outside of
/ext using the standards defined PRI macros.


We can do both, they are not alternatives:
- Number 2 is valuable because writing portable code leaves our
options open in the future.
- Number 1 is valuable because it will ensure consistent breakage under
/ext.

Ironically, that said, not doing 1) helps with 2) because it flushes
out broken code.

But either way, whether or not we do 1) I strongly advocate that for
our own code we adopt 2).

That sounds reasonable to me. Change minimal libc to align with newlib and
apply 2, the question is if we want to enforce usage of PRI macros.

Anas

Cheers
/Marcus

Hi,

On Fri, Jan 20, 2017 at 12:21 PM, Anas Nashif <nashif(a)gmail.com> wrote:

On Fri, Jan 20, 2017 at 5:26 AM, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote:

On 20 January 2017 at 04:06, Kumar Gala <kumar.gala(a)linaro.org> wrote:

As Anas said, I don’t think we can expect 3rd party software to utilize PRIu32. The fact that newlib and minimal libc different in the way the typedef (u)int32_t seems like a pointless difference for us to maintain and have to deal with a lot of pain if one switches between them.

This is not a choice between alternatives. There are two separate
decisions here:

1) Do we align minimal C with newlib in order to have consistent
breakage in /ext irrespective of the users choice between newlib and
minimal lib.

2) Do we write portable conforming code in our own tree outside of
/ext using the standards defined PRI macros.


We can do both, they are not alternatives:
- Number 2 is valuable because writing portable code leaves our
options open in the future.
- Number 1 is valuable because it will ensure consistent breakage under /ext.

Ironically, that said, not doing 1) helps with 2) because it flushes
out broken code.

But either way, whether or not we do 1) I strongly advocate that for
our own code we adopt 2).

That sounds reasonable to me. Change minimal libc to align with newlib and apply 2, the question is if we want to enforce usage of PRI macros.

A bit off topic, but could someone shed some light on why Zephyr has
support for two libc implementations?
ie Why not just always use newlib ?


--
BR
Szymon Janc