Date
1 - 20 of 34
uint32_t typedef differences causes issues
Kumar Gala
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
|
|
Marcus Shawcroft <Marcus.Shawcroft@...>
On 19 Jan 2017, at 14:41, Kumar Gala <kumar.gala(a)linaro.org> wrote: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.
|
|
Marcus Shawcroft <marcus.shawcroft@...>
Hi,
toggle quoted messageShow quoted text
[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
On 19 January 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:
|
|
Hi Kumar,
On Thu, Jan 19, 2017, Kumar Gala wrote: It looks like newlib and our mini libc define uint32_t differently andThe 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
|
|
Marcus Shawcroft <marcus.shawcroft@...>
On 19 January 2017 at 14:52, Johan Hedberg <johan.hedberg(a)intel.com> wrote:
Hi Kumar,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
|
|
Kumar Gala
On Jan 19, 2017, at 8:52 AM, Johan Hedberg <johan.hedberg(a)intel.com> wrote: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: Does "correct" in this case have any practical significance? It worsensThe right format specifier for unsigned integers is %u and not %d, so asSince the type is 'uint32_t' rather than 'unsigned' the correct specifier is: 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
|
|
Kumar Gala
On Jan 19, 2017, at 8:57 AM, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote: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
|
|
Marcus Shawcroft <marcus.shawcroft@...>
#include <stdint.h>Not having much luck there, but its possible I’m missing something obvious because of lack of sleep: That would be my fault, my example above is missing: #include <inttypes.h> Cheers /Marcus
|
|
Kumar Gala
On Jan 19, 2017, at 9:15 AM, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote:Ah, that fixes things.#include <stdint.h>Not having much luck there, but its possible I’m missing something obvious because of lack of sleep: Now to the question about how uint32_t should be defined ;) - k
|
|
Anas Nashif
On Thu, Jan 19, 2017 at 10:03 AM, Johan Hedberg <johan.hedberg(a)intel.com>
wrote: Hi Marcus,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
|
|
Marcus Shawcroft <marcus.shawcroft@...>
On 19 January 2017 at 15:39, Anas Nashif <nashif(a)gmail.com> wrote:
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: I'd just like to add to this that while working with Linux and BlueZOn Thu, Jan 19, 2017, Marcus Shawcroft wrote:We need to take into consideration 3rd party code that we can't modify, soasThe right format specifier for unsigned integers is %u and not %d, sospecifier is:far as I see that's the issue and using %u should make the error goSince the type is 'uint32_t' rather than 'unsigned' the correctPRIu32Does "correct" in this case have any practical significance? It worsens 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
|
|
Oliver Hahm <oliver.hahm@...>
Hi!
Generally I'm a big fan of doing things correctly & properly, but thisUnless you don't want to exclude the possibility to run Zephyr on 8-bit and 16-bit architectures. Cheers, Oleg
|
|
Boie, Andrew P
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.That would be my fault, my example above is missing:Ah, that fixes things. Newlib and minimal libc both have their own inttypes.h which are defined properly for that C library. Andrew
|
|
Kumar Gala
On Jan 19, 2017, at 11:35 AM, Boie, Andrew P <andrew.p.boie(a)intel.com> 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.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.That would be my fault, my example above is missing:Ah, that fixes things. For example, trying building by setting CONFIG_NEWLIB_LIBC=y in tests/include/test.config and see how much breaks. - k
|
|
Marcus Shawcroft <marcus.shawcroft@...>
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
|
|
Tomasz Bursztyka
Hi Marcus,
On 20 January 2017 at 04:06, Kumar Gala <kumar.gala(a)linaro.org> wrote:I'd rather stick with 1). User PRI for all uint32_t is going to be ugly.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 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
|
|
Anas Nashif
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:That sounds reasonable to me. Change minimal libc to align with newlib andAs Anas said, I don’t think we can expect 3rd party software to utilizePRIu32. The fact that newlib and minimal libc different in the way the apply 2, the question is if we want to enforce usage of PRI macros. Anas
|
|
Szymon Janc <ext.szymon.janc@...>
Hi,
On Fri, Jan 20, 2017 at 12:21 PM, Anas Nashif <nashif(a)gmail.com> wrote: 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
|
|