Warnings about comparison between signed and unsigned values are broken and can't be easily restored (IDFGH-12121)

[ScumCoder]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

ESP-IDF version:
Latest stable (5.1.2)
Steps to reproduce:
Try to build the following MWE:

#include <iostream>
#include <limits>

void LessThan(uint64_t a, int64_t b)
{
    if(a < b)
        std::cout << a << " < " << b << "\n";
    else
        std::cout << a << " >= " << b << "\n";
}

extern "C" void app_main(void)
{
    LessThan(1, std::numeric_limits<int64_t>::lowest());
}

with the following commands added to CMakeLists.txt:

idf_build_set_property(COMPILE_OPTIONS "-Wextra" APPEND)
idf_build_set_property(COMPILE_OPTIONS "-Wall" APPEND)
idf_build_set_property(COMPILE_OPTIONS "-Werror" APPEND)

Expected result:
The compilation process crashes with the following error:

error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     if(a < b)
        ~~^~~

Actual result:
The program builds without any warnings.
If flashed to an uC (ESP32), the monitor shows

1 < -9223372036854775808

This is already a very serious issue, because GCC documentation states that specifying both -Wall and -Wextra must guarantee that -Wsign-compare is going to be enabled both for C and C++. But that's not the worst part.

Specifying

idf_build_set_property(COMPILE_OPTIONS "-Wsign-compare" APPEND)

in CMakeLists.txt actually enables the proper warning behavior. However, then it becomes impossible to compile the project because the FreeRTOS source code itself contains violations of the comparison signedness correctness; the compilation process crashes with the following error:

In file included from /home/user/esp/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h:74,
                 from /home/user/esp/esp-idf/components/freertos/FreeRTOS-Kernel/include/freertos/portable.h:59,
                 from /home/user/esp/esp-idf/components/freertos/FreeRTOS-Kernel/include/freertos/FreeRTOS.h:71,
                 from /home/user/esp/esp-idf/components/esp_system/include/esp_task.h:24,
                 from /home/user/esp/esp-idf/components/lwip/port/include/lwipopts.h:20,
                 from /home/user/esp/esp-idf/components/lwip/lwip/src/include/lwip/opt.h:51,
                 from /home/user/esp/esp-idf/components/lwip/lwip/src/include/lwip/sockets.h:42,
                 from /home/user/esp/esp-idf/components/lwip/include/lwip/sockets.h:8,
                 from /home/user/esp/esp-idf/components/lwip/port/esp32xx/include/sys/socket.h:15,
                 from /home/user/esp/esp-idf/components/mbedtls/mbedtls/library/x509_crt.c:2713:
/home/user/esp/esp-idf/components/esp_hw_support/include/spinlock.h: In function 'spinlock_acquire':
/home/user/esp/esp-idf/components/esp_hw_support/include/spinlock.h:117:94: error: comparison of integer expressions of different signedness: 'esp_cpu_cycle_count_t' {aka 'long unsigned int'} and 'int32_t' {aka 'long int'} [-Werror=sign-compare]
  117 |     } while ((timeout == SPINLOCK_WAIT_FOREVER) || (esp_cpu_get_cycle_count() - start_count) <= timeout);
      |                                                                                              ^~
cc1: all warnings being treated as errors

[M-Bab]

Can confirm this unwanted behavior.

Stumbling on the same problem at the moment. The issue checklist actually prevented me from opening a duplicate 😉

So the offending line is: https://github.com/espressif/esp-idf/blob/master/components/esp_hw_support/include/spinlock.h#L135

This is broken since IDF 5.0 (==the existence of the spinlock.h file) so it affects 5.0, 5.1, 5.2, 5.3, 5.4, develop.

the FreeRTOS source code itself

This is misleading: The spinlock.h is in 100% control and responsibility of Espressif. However, it has some interaction with FreeRTOS via the portable implementation. Intuitively I would say that timeout should be esp_cpu_cycle_count_t as well with SPINLOCK_WAIT_FOREVER=UINT32_MAX (==-1 ABI compatible) but this would be a bit more complicated.

So the easy fix - doing a cast before the compare - remains:

diff --git a/components/esp_hw_support/include/spinlock.h b/components/esp_hw_support/include/spinlock.h
index 7501faabea..fb4585b41e 100644
--- a/components/esp_hw_support/include/spinlock.h
+++ b/components/esp_hw_support/include/spinlock.h
@@ -132,7 +132,7 @@ static inline bool __attribute__((always_inline)) spinlock_acquire(spinlock_t *l
             break;
         }
         // Keep looping if we are waiting forever, or check if we have timed out
-    } while ((timeout == SPINLOCK_WAIT_FOREVER) || (esp_cpu_get_cycle_count() - start_count) <= timeout);
+    } while ((timeout == SPINLOCK_WAIT_FOREVER) || (esp_cpu_get_cycle_count() - start_count) <= ((esp_cpu_cycle_count_t) timeout));
 
 exit:
     if (lock_set) {

If you fix this can you please backport it to all 5.x versions? It would be great, if I don't need to weaken my build configuration for this header.

@Harshal5 you did such a great and fast job last time - do you think you can help out here as well?

[igrr]

Also linking #11239 which will fix the fact that the warning is not enabled in IDF by default in the first place.

[KonstantinKondrashov]

Hi @ScumCoder @M-Bab!
I will help to fix the warnings. Thanks for the report. Yes, I think we can backport it to v5.x as well.

[KonstantinKondrashov]

#14469