Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IDF 5.0] type 'uint32_t' {aka 'long unsigned int'} (IDFGH-8001) #9511

Closed
chegewara opened this issue Aug 6, 2022 · 53 comments
Closed

[IDF 5.0] type 'uint32_t' {aka 'long unsigned int'} (IDFGH-8001) #9511

chegewara opened this issue Aug 6, 2022 · 53 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@chegewara
Copy link
Contributor

chegewara commented Aug 6, 2022

Im not sure if its possible to reproduce with normal example, but i am trying to add managed component to my custom component as dependency (here it is USB host as example, but it was also with button). I think error log is self explaining the problem:

Executing action: all (aliases: build)
Running cmake in directory /home/chegewara/upwork/lvgl_demo/demo2_fail/build
Executing "cmake -G Ninja -DPYTHON_DEPS_CHECKED=1 -DESP_PLATFORM=1 -DIDF_TARGET=esp32s3 -DCCACHE_ENABLE=0 /home/chegewara/upwork/lvgl_demo/demo2_fail"...
-- Found Git: /usr/bin/git (found version "2.25.1") 
-- Component directory /home/chegewara/esp/master/components/asio does not contain a CMakeLists.txt file. No component will be added
-- Component directory /home/chegewara/esp/master/components/coap does not contain a CMakeLists.txt file. No component will be added
-- Component directory /home/chegewara/esp/master/components/expat does not contain a CMakeLists.txt file. No component will be added
-- Component directory /home/chegewara/esp/master/components/nghttp does not contain a CMakeLists.txt file. No component will be added
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /home/chegewara/.espressif/tools/xtensa-esp32s3-elf/esp-2022r1-RC1-11.2.0/xtensa-esp32s3-elf/bin/xtensa-esp32s3-elf-gcc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/chegewara/.espressif/tools/xtensa-esp32s3-elf/esp-2022r1-RC1-11.2.0/xtensa-esp32s3-elf/bin/xtensa-esp32s3-elf-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/chegewara/.espressif/tools/xtensa-esp32s3-elf/esp-2022r1-RC1-11.2.0/xtensa-esp32s3-elf/bin/xtensa-esp32s3-elf-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Project is not inside a git repository, or git repository has no commits; will not use 'git describe' to determine PROJECT_VER.
-- Building ESP-IDF components for target esp32s3
Solving dependencies requirements
Updating lock file at /home/chegewara/upwork/lvgl_demo/demo2_fail/dependencies.lock
Processing 2 dependencies:
[1/2] espressif/usb_host_cdc_acm (1.0.0)
[2/2] idf (5.1.0)

...

/home/chegewara/upwork/lvgl_demo/demo2_fail/managed_components/espressif__usb_host_cdc_acm/cdc_acm_host.c: In function 'cdc_acm_host_line_coding_get':
/home/chegewara/esp/master/components/log/include/esp_log.h:265:27: error: format '%d' expects argument of type 'int', but argument 6 has type 'uint32_t' {aka 'long unsigned int'} [-Werror=format=]

The question is why uint32_t is of type long unsigned int?
With v4.4 it is building without problems.

Thanks

EDIT

HINT: The issue is better to resolve by replacing format specifiers to 'PRI'-family macros (include <inttypes.h> header file).
@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 6, 2022
@github-actions github-actions bot changed the title [IDF 5.1.0] type 'uint32_t' {aka 'long unsigned int'} [IDF 5.1.0] type 'uint32_t' {aka 'long unsigned int'} (IDFGH-8001) Aug 6, 2022
@chegewara
Copy link
Contributor Author

duplicate #6906

@xobs
Copy link
Contributor

xobs commented Aug 7, 2022

I started getting errors with the latest nightly. After bisecting, it seems as though d10d57a is the offending commit that broke things.

I think d10d57a should be reverted until #6906 is fixed.

@xobs
Copy link
Contributor

xobs commented Aug 7, 2022

Note that as a workaround, you can add this to your CMakeLists.txt until either #9511 or #6906 are fixed:

component_compile_options(-Wno-error=format= -Wno-format)

@igrr
Copy link
Member

igrr commented Aug 7, 2022

Sorry for the breakage folks, we meant to silence the formatting errors in all Espressif-maintained components before merging d10d57a, but apparently have missed those two. Will fix soon.

I think #6906 will be closed with resolution "works as intended" now that we have uint32_t == unsigned long for both Xtensa and RISC-V.

Linking also the relevant part of the v5.0 migration guide: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/migration-guides/release-5.x/gcc.html#int32-t-and-uint32-t-for-xtensa-compiler

@chegewara
Copy link
Contributor Author

Not a big problem, i just wanted to play around, and as usually i am so unlucky and i picked 2 components which are the only affected by the issue.

@xobs
Copy link
Contributor

xobs commented Aug 7, 2022

I ran into this issue because my project was depending on another project which doesn't compile with the new change.

The workaround I posted can be used while you wait for upstream projects to merge fixes.

igrr added a commit to espressif/idf-extra-components that referenced this issue Aug 7, 2022
@igrr igrr changed the title [IDF 5.1.0] type 'uint32_t' {aka 'long unsigned int'} (IDFGH-8001) [IDF 5.0] type 'uint32_t' {aka 'long unsigned int'} (IDFGH-8001) Aug 7, 2022
@AxelLin
Copy link
Contributor

AxelLin commented Aug 8, 2022

@igrr @BrianPugh

With current esp-idf master tree, I got many build error in esp_littlefs:
e.g.
managed_components/joltwallet__littlefs/src/esp_littlefs.c:1747:9: note: in expansion of macro 'ESP_LOGV'
1747 | ESP_LOGV( TAG, "Truncated file %s to %u bytes", file->path, (uint32_t) size );
| ^~~~~~~~

Also note I got similar errors in my application code. (Mostly in ESP_LOGx)
I hope my application can be compiled with master tree and stable branches, what's your suggestion to fix this build issue?

@BrianPugh
Copy link
Contributor

some of my logging was a little loose on types; fixing them right now and will make a release.

@AxelLin
Copy link
Contributor

AxelLin commented Aug 8, 2022

some of my logging was a little loose on types; fixing them right now and will make a release.

@BrianPugh
Thanks for the quick fix: joltwallet/esp_littlefs@926b46f
I'm wondering if using PRIu32 to replace %u for uint32_t is better than cast the uint32_t variables to unsigned int.

@AxelLin
Copy link
Contributor

AxelLin commented Aug 8, 2022

@igrr

Is it possible to keep "-Wno-error=format=" for a while so the out-of-tree modules have chance to fix this compile warnings.
People may use some out-of-tree modules not maintained by the IDF component registry.

@igrr
Copy link
Member

igrr commented Aug 8, 2022

@AxelLin We could do that, however as far as I can tell from littlefs and a few other projects, CI builds aren't using -Werror flag, so the new warnings would mostly go unnoticed.

For any component which doesn't compile with -Werror yet, you can add the following into the project CMakeLists.txt file, after the project() line:

# e.g. for espressif/button component:
idf_component_get_property(lib espressif__button COMPONENT_LIB)
target_compile_options(${lib} PRIVATE -Wno-error=format)

(optionally after checking that IDF version >= 5.0)

Regarding the original issue report, usb_host_cdc_acm has also been updated with the fix.

@igrr igrr pinned this issue Aug 8, 2022
@davidzuhn
Copy link
Contributor

I think that this change will affect a lot of code. This was especially painful on my part, as about the only place I use printf style formatting is in ESP_LOGx, and the way the macros get defined will bury the actual location of the error under a number of entries shown for esp_log.h.

It is, however, the right change to make and will result in better quality code.

@franz-ms-muc
Copy link
Contributor

I can confirm:
inttypes.h
solve this Problem.
especially for mixed 4.0/5.0 Environments.

see Sample here:

@franz-ms-muc
Copy link
Contributor

I can confirm: inttypes.h solve this Problem. especially for mixed 4.0/5.0 Environments.

see Sample here:

espressif/esp-protocols@71401a0

ankrysm added a commit to ankrysm/esp32_ws2812 that referenced this issue Aug 22, 2022
@bobbygz
Copy link

bobbygz commented Aug 22, 2022

Ran into this trying to build the tot-reference-esp32c3 for esp32-s3.

From the comment above: "I think #6906 will be closed with resolution "works as intended" now that we have uint32_t == unsigned long for both Xtensa and RISC-V"

It appears that xtensa still has issues with unsigned long (c string specifiers in the logging calls).

Thanks for the workaround!!!

@fpgamaster
Copy link

Hmmm, it is insane!
What is the result of sizeof(uint32_t) ?

@igrr
Copy link
Member

igrr commented Aug 24, 2022

@fpgamaster sizeof(uint32_t) == sizeof(unsigned int) == sizeof(unsigned long) for both the older toolchain releases and the new release. However C language distinguishes int and long types, even if they have the same size.

If you consider the following code,

#include <stdint.h>
#include <stdio.h>
void print(int32_t num) {
    printf("My %x\n", num);
}

On several architectures, compiler will produce an error:

In function 'void print(int32_t)':
error: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'int32_t' {aka 'long int'} [-Werror=format=]
    4 |     printf("My %x\n", num);
      |                ~^     ~~~
      |                 |     |
      |                 |     int32_t {aka long int}
      |                 unsigned int
      |                %lx
cc1plus: all warnings being treated as errors
Compiler returned: 1

This is because int32_t is defined as long int, and the correct format string for long int is %lx. (By correct I mean the one defined by the standard and expected by the compiler; the format string %x will still work, if you silence the warning.)

This behavior is not unique to GCC for Xtensa architecture. Here you can see that the same error is produced by GCC for the 32-bit ARM: https://godbolt.org/z/Yfb1Tdbbc and for the 32-bit RISC-V: https://godbolt.org/z/ras4bvf86.
As I mentioned above, we have changed the definition of uint32_t for Xtensa from unsigned int to unsigned long to match the way it is defined in upstream GCC, and also for the other architectures.

It would have been nice if GCC had a command line option to disable such "pedantic" format string warnings while keeping the "useful" format string warnings. There is a request for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80060.

We have decided that while the breakage caused by changing the uint32_t type is going to be painful in near term, longer term it is better to have the format warnings enabled, as these warnings often catch legitimate bugs. We are gradually fixing the format warnings in ESP-IDF components and removing the -Wno-format flags, and we hope that the developers building projects and components with ESP-IDF will do the same.

@fpgamaster
Copy link

Hi @igrr,
Thanks for the replay!

Ok, But the size of int and unsigned int is 4 bytes, respectively long int is 8 bytes.
If sizeof(uint32_t) == sizeof(unsigned int) == sizeof(unsigned long) then does it mean that the size of uint32_t is 8 bytes !?

'unsigned long' stands for 'unsigned long int'?
What I am missing here?

@igrr
Copy link
Member

igrr commented Aug 24, 2022

Ok, But the size of int and unsigned int is 4 bytes, respectively long int is 8 bytes.

If we are talking about 32-bit RISC-V, Xtensa (or ARM, for that matter), that's not the case: both int and long are 4 bytes long.

long is typically 8 bytes long on 64-bit architectures.

You can check this for various compilers here: https://godbolt.org/z/hzbjMKKrs. (To make the size visible at compile time, the code produces a compiler error. The size can be seen as part of the error message on the right, e.g. the 4 in char (*)[4].) If you switch to a 64-bit architecture (like RV64GC or ARM64) you will see the size of long changing to 8.

@fpgamaster
Copy link

@igrr,
The thing I care of is how the data is stored in memory.
You are saying that sizeof(uint32_t) == sizeof(unsigned int) == sizeof(unsigned long)
So, if I have a variable which is uint32_t and a variable which is unsigned long, in how many bytes they will be stored on an ESP32?
Also, If I have a pointers to such variables and if I increment them by one, what will be the result?

Best regards

@igrr
Copy link
Member

igrr commented Aug 24, 2022

So, if I have a variable which is uint32_t and a variable which is unsigned long, in how many bytes they will be stored on an ESP32?

Both variables will occupy 4 bytes, and incrementing the pointers to such variables will move the pointer by 4 bytes. It is the same for all of these: sizeof(unsigned int) == 4, sizeof(unsigned long) == 4, sizeof(uint32_t) == 4.

If you were compiling the code for a 64-bit architecture, you would typically see
sizeof(unsigned int) == 4, sizeof(unsigned long) == 8, sizeof(uint32_t) == 4.

@fpgamaster
Copy link

@igrr,
Ok, for what the keyword 'long' stands for on a 32-bits platforms?

Best regards

@igrr igrr closed this as completed Dec 16, 2022
@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally and removed Status: Opened Issue is new labels Dec 16, 2022
@franz-ms-muc
Copy link
Contributor

franz-ms-muc commented Dec 16, 2022

Is this not solved yet @igrr

@mallaprashant please look into the latest hello World:

https://github.com/espressif/esp-idf/blob/master/examples/get-started/hello_world/main/hello_world_main.c

the Printf must look like this:

printf("Minimum free heap size: %" PRIu32 " bytes\n", esp_get_minimum_free_heap_size());

@FlynnMa
Copy link

FlynnMa commented Dec 17, 2022

Why not use standard type 'uint32_t' which is a common definition coss all system. 'PRIu32' seems a duplicated definition introduce conflicts and complicity.

@baldhead69
Copy link

PRIu32 is for printing formating to uint32_t variables.

@FlynnMa
Copy link

FlynnMa commented Dec 17, 2022

uint32_t is formating to 'unsigned int', that's the compiler needs, why do we do a formating fron 'PRIu32' -> 'uint32_t'?

@baldhead69
Copy link

PRIu32 is string.

uint32_t is a binary number.

The printf transform de binary number in a string for better human readability.

Example:

uint32_t a = 0x12345678

To store this number in memory the computer need 4 bytes(each byte in a single memory address).
In little endian format stay:

Address 0: 0x78
Address 1: 0x56
Address 2: 0x34
Address 3: 0x12

The 'PRIu32' instruct printf to decode these 4 bytes in a string.

Dont confuse string with number.
See ascii table.

That's more or less it.

@baldhead69
Copy link

Not all compilers consider unsigned int 32 bits.

This varies by compiler and hardware platform.

Microchip's xc16 compiler, for example, considers unsigned int to be 16 bits.

@FlynnMa
Copy link

FlynnMa commented Dec 17, 2022

OK, I see.

So PRIu32 is a string for "%d"? But I don't see any needs to introduct PRIu32 as a string for printf. It looks to me the fix is to hide the warning instead of fix it.

'int' is dynamic according with CPU architecture, specificaly it is the data bus width between CPU and it's memory. As I mentioned above, to eliminate the dynamic we want to use 'uint32_t' or 'uint16_t' instead of working with 'int' 'short' directly.

So we tell our definition which CPU architecture we are working on, meanwhile tell c compiler what is the targeted CPU architcture, that's all we need to do.

@baldhead69
Copy link

PRIu32 is a formatting string for uint32_t.

PRId32 is a formatting string for int32_t.

https://www.ibm.com/docs/en/zos/2.4.0?topic=files-inttypesh-io-format-integer-types

@FlynnMa
Copy link

FlynnMa commented Dec 17, 2022

OK, that is fine...
But my point is uint32_t should be defined as 'unsigned int', the error says it is defined as " long unsigned int", see bellow:

/home/chegewara/esp/master/components/log/include/esp_log.h:265:27: error: format '%d' expects argument of type 'int', but argument 6 has type 'uint32_t' {aka 'long unsigned int'} [-Werror=format=]

Which means uint32_t is wrongly defined as 64bits long?

@igrr
Copy link
Member

igrr commented Dec 17, 2022

But my point is uint32_t should be defined as 'unsigned int', the error says it is defined as " long unsigned int", see bellow:

In this case uint32_t is defined as unsigned long. long is still defined as a 32-bit type, so uint32_t is also 32-bit.
Here's an example: https://godbolt.org/z/KKbfd1Kzf. You will see the same behavior for 32-bit ARM and RISC-V ("none" a.k.a bare-metal). I've written more on this in this comment above: #9511 (comment)

@chegewara
Copy link
Contributor Author

I dont complain, but this is misleading:

error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'int'

According to this error argument is int, but:

ESP_LOGI(TAG, "Cpu frequecny is %" PRIu32 "", CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ); <--- this does not compile
ESP_LOGI(TAG, "Cpu frequecny is %" PRIu16 "", CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ); <--- this compile fine

So, is int 16 or 32 bits?

Thanks

@igrr
Copy link
Member

igrr commented Dec 17, 2022

int is 32 bits. long is also 32 bits. short is 16 bits.

The safest way to check the real size of the type is to use sizeof, e.g.: _Static_assert(sizeof(uint32_t) == 4);


ESP_LOGI(TAG, "Cpu frequency is %" PRIu32 "", CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ); <--- this does not compile
ESP_LOGI(TAG, "Cpu frequency is %" PRIu16 "", CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ); <--- this compile fine

This is indeed confusing, I agree. I'll try to explain why this happens.

First, the macro CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ is expanded to an integer value, for example 160.

In C, variadic arguments undergo default argument promotions. Integer types with rank lower or equal than int are promoted to int or unsigned int (details). This means that if you were to pass a short (16-bit) value to printf, it would actually get promoted to a 32-bit int first, and the variadic function (printf) would receive a 32 value.

To compensate for this promotion of the argument, C language standard defines the h length modifier as follows:

Specifies that a following b, d, i, o, u, x, or X conversion specifier applies to a short int or unsigned short int argument (the argument will have been promoted according to the integer promotions, but its value shall be converted to short int or unsigned short int before printing);

If you were to write the following code:

unsigned short val = 160;
printf("%hu\n", val);

the 16-bit value val would first get promoted to a 32-bit unsigned int, passed to printf, and then printf would convert it back to unsigned short before printing.

Now consider the case if val is unsigned int instead:

unsigned int val = 160;
printf("%hu\n", val);

The exact same thing happens as in the previous case: val gets promoted to unsigned int (which doesn't change anything, as it is already an unsigned int), then printf converts unsigned int to unsigned short before printing.

This code compiles and works, and the compiler doesn't warn about the mismatch between unsigned int and %hu. Could have the compiler warned about this case? I'm not a compiler engineer, so it's hard for me to tell. I guess the reason why it's not very easy to produce a warning in this case is that printf-specific argument type analysis in the compiler happens after the integer promotions are done.

At the same time, the following code:

unsigned long val = 160;
printf("%u\n", val);

generates a warning. This is because unsigned long has higher rank than unsigned int, so it does not get promoted to unsigned int. This means that printf receives unsigned long argument, and the compiler is able to detect that.

The important thing to understand about these conversions and warnings is that they are all related to integer type ranks, not integer type sizes. The compiler treats int and long as different types, even if they have the same size and representation in memory.

Related to this topic, there is a feature request for GCC to provide a variant of -Wformat flag which would not warn about type mismatch when types have the same width: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80060.

@chegewara
Copy link
Contributor Author

I know the int is 32 bits, but what i meant is notation confusing PRIu16 vs PRIu32, but thanks for this explanation. Its always good to understand better some mechanisms we dont think about usually.

Thanks

@igrr
Copy link
Member

igrr commented Dec 17, 2022

So, is int 16 or 32 bits?

I know the int is 32 bits

Sorry, maybe I misunderstood your question.

Did you mean "what format specifier should i use to format an int"?

If the variable is declared as an int (not as a fixed-width integer type), you don't have to use PRI* macros at all. Depending on the type, you should use the following format specifiers:

  • int: %d or %i
  • unsigned int: %u or %x
  • short: %hd
  • unsigned short: %hu or %hx
  • long: %ld
  • unsigned long: %lu or %lx
  • int32_t: "%" PRId32
  • uint32_t: "%" PRIu32 or "%" PRIx32
  • int16_t: "%" PRId16
  • uint16_t: "%" PRIu16 or "%" PRIx16

(See cppreference for the full list)

@murarduino
Copy link

I would like to ask, which version of the fix this BUG, I am now using 5.1.1, whether it is %x or %d will show incorrect type.
ex:
ESP_LOGD("AUX_FUNC", "fade_on end: %x", xTaskGetTickCount());

error: format '%x' expects argument of type 'unsigned int', but argument 6 has type 'TickType_t' {aka 'long unsigned int'} [-Werror=format=]
69 | ESP_LOGD("AUX_FUNC", "fade_off start: %x", xTaskGetTickCount());
| ~
| |
| TickType_t {aka long unsigned int}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests