portGET_ARGUMENT_COUNT() defintion broken for C++ (IDFGH-4021)

[seijikun]

Environment

• Development Kit: none
• Module or chip used: ESP32-WROOM-32
• IDF version (run git describe --tags to find it): v4.3-dev-1197-g8bc19ba89
• Build System: idf.py
• Compiler version (run xtensa-esp32-elf-gcc --version to find it): xtensa-esp32-elf-gcc (crosstool-NG esp-2020r3) 8.4.0
• Operating System: Linux
• Using an IDE?: No
• Power Supply: USB

Problem Description

The macro portGET_ARGUMENT_COUNT() in portmacro.h line 331 seems to be broken when used with C++.
Have a look at the demonstration here. With 0 arguments, it returns 1 instead of 0. This triggers the first static assertion directly below the definition:

_Static_assert(portGET_ARGUMENT_COUNT() == 0, "portGET_ARGUMENT_COUNT() result does not match for 0 arguments");

As can be seen here, this works when instead run with C.

Code to reproduce this issue

#include <iostream>

#define portGET_ARGUMENT_COUNT(...) portGET_ARGUMENT_COUNT_INNER(0, ##__VA_ARGS__,1,0)
#define portGET_ARGUMENT_COUNT_INNER(zero, one, count, ...) count

int main() {
    std::cout << portGET_ARGUMENT_COUNT() << std::endl;
    std::cout << portGET_ARGUMENT_COUNT(1) << std::endl;
    std::cout << portGET_ARGUMENT_COUNT(1,2) << std::endl;
    std::cout << "###" << std::endl;

    return 0;
}

Log

I am using a main.cpp in my main component. The include tree looks as follows:

In file included from <esp32_path>/tools/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/xtensa-esp32-elf/sys-include/stdlib.h:19,
                 from <esp32_path>/esp32/tools/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/xtensa-esp32-elf/include/c++/8.4.0/cstdlib:75,
                 from <esp32_path>/esp32/tools/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/xtensa-esp32-elf/include/c++/8.4.0/stdlib.h:36,
                 from <esp32_path>/esp32/esp-idf/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h:122,
                 from <esp32_path>/esp32/esp-idf/components/freertos/include/freertos/FreeRTOS.h:101,
                 from ../main/main.cpp:9:
                 
<esp32_path>/esp-idf/components/freertos/xtensa/include/freertos/portmacro.h:334:41: error: static assertion failed: portGET_ARGUMENT_COUNT() result does not match for 0 arguments
 _Static_assert(portGET_ARGUMENT_COUNT() == 0, "portGET_ARGUMENT_COUNT() result does not match for 0 arguments");

Am I doing something wrong here?

[atanisoft]

This also breaks portYIELD_FROM_ISR(); when called from C++ code.

[atanisoft]

Here is a working portGET_ARGUMENT_COUNT() (named as portGET_ARGUMENT_COUNT_ALT). Tested with C, C++, C++11, C++17.

[Alvin1Zhang]

Thanks for reporting, we will look into.

[0xjakob]

@seijikun The code you posted seems to be for x86/x64 gcc? In IDF, the main function is called void app_main or extern "C" void app_main in case of C++. I changed the code to work on the ESP32 as C++ code, however and the output seems just fine:

0
1
2
###

The example you posted is C++17 code, while IDF currently always uses C++11. It seems not very likely but maybe that's the reason. Could you try to change the reproducing code so that it can be compiled for the ESP32 while exposing the error?

[seijikun]

The code you posted seems to be for x86/x64 gcc

Yes. I thought, that the preprocessor should work the same everywhere.

Here is my simplified project, where the error occurs:
example.tar.gz
It's currently configured to build with C++17, but changing it to C++11 has the same effect.

I am building with ESP-IDF Master (8bc19ba893e5544d571a753d82b44a84799b94b1):

 f31694c9f1746ba189a4bcae2e34db15135ddb22 components/asio/asio (asio-1-12-0-201-gf31694c9)
 d037ec89546fad14b5c4d5456c2e23a71e554966 components/bootloader/subproject/components/micro-ecc/micro-ecc (v1.0)
 a799b1910995a2d7129e8cf02549facc18af186f components/bt/controller/lib (remotes/origin/HEAD)
 95bd8644abf4a410dd3fb914468d3a23ac9bbec2 components/bt/host/nimble/nimble (heads/nimble-1.2.0-idf)
 085ca40781f7c39febe6d14fb7e5cba342e1804b components/cbor/tinycbor (v0.5.3-6-g085ca40)
 eeecc49ce8af123cf8ad40efdb9673e37b56230f components/cmock/CMock (v2.5.2-2-geeecc49)
 98954eb30a2e728e172a6cd29430ae5bc999b585 components/coap/libcoap (v4.2.0-rc4-114-g98954eb)
 41abe309140a05939bda776b7d1a744f76612582 components/esp_wifi/lib (heads/master-1-g41abe30)
 10225816df4c9c1b078e677e8a75a87778786ad1 components/esptool_py/esptool (v2.8-61-g1022581)
 968b8cc46dbee47b83318d5f31a8e7907199614b components/expat/expat (R_2_2_5)
 3c8935676a97c7c97bf006db8312875b4f292f6c components/json/cJSON (v1.7.12)
 70170c28c844a4786e75efc626e1aeebc93caebc components/libsodium/libsodium (1.0.12)
 80d6d19a929c6db577fc44a47fdb4acffdd68933 components/lwip/lwip (remotes/origin/master-199-g80d6d19a)
 90f46c8b17bc1219a82d4ddf81520d40c5ac5ebf components/mbedtls/mbedtls (mbedtls-2.13.1-1599-g90f46c8b1)
 01594bf118ae502b5a0ead040446f2be75d26223 components/mqtt/esp-mqtt (ESP-MQTT_FOR_IDF_3.1-192-g01594bf)
 8f7b008b158e12de0e58247afd170f127dbb6456 components/nghttp/nghttp2 (v1.41.0)
 dac1a65feac4ad72f612aab99f487056fbcf5c1a components/protobuf-c/protobuf-c (v1.3.0)
 f5e26c4e933189593a71c6b82cda381a7b21e41c components/spiffs/spiffs (0.2-221-gf5e26c4)
 a2ba3dccccf94022d31e939fa2ce4dca5f0a34f0 components/tinyusb/tinyusb (remotes/origin/esp-based_on_57ffa94a)
 7d2bf62b7e6afaf38153041a9d53c21aeeca9a25 components/unity/unity (v2.4.3-51-g7d2bf62)
 7e8e249990ec491ec15990cf95b6d871a66cf64a examples/build_system/cmake/import_lib/main/lib/tinyxml2 (2.0.2-659-g7e8e249)
 71e69383a0368bb704e1b0d90b7e5697afae7d5d examples/peripherals/secure_element/atecc608_ecdsa/components/esp-cryptoauthlib (heads/master)

[0xjakob]

@seijikun Thanks! I can reproduce the error. Will have a look. Please keep in mind that we don't currently support any other standard than C++11.

[0xjakob]

@seijikun I switched it back to C++11 and it works (by commenting the line in the CMakeLists.txt). When I switch it to C++17, it fails again. Could you verify that this also occurs with C++11?

[seijikun]

@0xjakob When I completely remove the component_compile_options(-std=c++17) line from the CMakeLists.txt, it works.
Changing the line to component_compile_options(-std=c++11), also fails to compile.

[0xjakob]

Ahh, sorry! We actually use -std=gnu++11 and this is what we test with. Interesting. Are you able to use the gnu++11 standard for your projects? When I explicitly set -std=c++11, I also experience this error.

[seijikun]

Ah! component_compile_options(-std=gnu++11) and component_compile_options(-std=gnu++17) indeed seem to work.
Thanks!
I think this might be the description of the differing behavior causing this error.

[igrr]

This probably means that we don't need to support the variadic form of portYIELD_FROM_ISR when compiling C++ code without GNU extensions. I think there should be a macro set by the compiler which we can check to determine if GNU extensions are enabled.

Edit: disregard, I have missed #5897 (comment).

[atanisoft]

@igrr is there a need to have the variadic form outside of the test compilation mode? I'm thinking not since all usages of the macro outside of the isr latency test are of the simple no argument form.

Also the snippet I provided above that works for various c++ versions is quite ugly as it pollutes the global namespace with a number of extra defines. It would be best to centralize that with include guards to prevent redefinition warnings and general namespace pollution.

[igrr]

@atanisoft No need in IDF, but we made this improvement to make such logic unnecessary for third party projects: https://github.com/hathach/tinyusb/blob/7d2fc124452d6ec2d5d45bdacc4fc8b38d03d0d0/src/osal/osal_freertos.h#L70-L74.

[atanisoft]

@igrr There are a few macros like this that are inconsistent between ports... Another is portENTER_CRITICAL where most ports do not take an argument, perhaps that can also be similarly fixed using a core specific portMUX_TYPE when no args are provided? That would cleanup code like this which has an ESP32 specific form due to requiring the portMUX_TYPE arg (ignore the ISR context check as that would be abstracted out using portENTER_CRITICAL_SAFE/portEXIT_CRITICAL_SAFE)

[igrr]

Another is portENTER_CRITICAL where most ports do not take an argument, perhaps that can also be similarly fixed using a core specific portMUX_TYPE when no args are provided?

That's a good point, it looks feasible at first glance. Would you mind opening a new issue so that we can track this?

[atanisoft]

Would you mind opening a new issue so that we can track this?

#5909

[qt1]

This seems also to happen with current arduino-esp32 .
Is there a recommended workaround ?

[atanisoft]

This seems also to happen with current arduino-esp32 .

Are you trying to use arduino-esp32 1.0.4 (or 1.0.5) against IDF 4.x? If so, it won't work. You will need to move up to the arduino-esp32 esp32s2 branch and use IDF 4.2+.

[qt1]

I think am using the latest arduinoesp32 based on idf 4.2.

I have just written a little utility to check revisions and compatibility:

#!/bin/bash
echo components/arduino revision is:
git -C components/arduino rev-parse --short HEAD

echo
echo arduino revision and esp-idf revision should match:
git -C $IDF_PATH rev-parse --short HEAD
awk  '/CONFIG_ARDUINO_IDF_COMMIT/{gsub("\"","");print $3}' < components/arduino/tools/sdk/esp32/include/config/sdkconfig.h

Result:

components/arduino revision is:
93c62261

arduino revision and esp-idf revision should match:
494a124d9
494a124d9

[atanisoft]

Are you on the esp32s2 branch or another branch of arduino-esp32?

[qt1]

I do not see a different branches for esp32 or esp32-s2 .
My project is an old esp32 project. Switched to 4.0 some time ago, and it should stay esp32..

Could you please provide commit number or check the commit number above?
The commit number should be more the most accurate pointer into the git revision tree.
Thanks

[atanisoft]

The esp32s2 branch is compatible for both esp32 and esp32s2, it has since been renamed though: https://github.com/espressif/arduino-esp32/tree/idf-release/v4.2

[qt1]

Yes, commit 93c6226 is the latest commit on the above - 2 dyes ago.
You can see that on the above page, the first line of the changes table.

[atanisoft]

@qt1 make sure you are using the GNU extensions for C++11 or later, ie:

string(REPLACE "-std=gnu++11" "-std=gnu++14" CXX_OPTIONS "${CXX_COMPILE_OPTIONS}")
idf_build_set_property(CXX_COMPILE_OPTIONS "${CXX_OPTIONS}" REPLACE)

There is an issue with using -std=c++14 (or any version really).

[qt1]

@atanisoft

It seems like the code uses an old gcc hack namely x , ## __VA_RAGS__
it should be using the new and portable __VA_OPT__(,) macro.

IMHO forcing the user to a specific version just to bypass a bug sounds kind of wrong..

In any case, it seems like the newest version v4.2 does not include either macros.

I would suggest to update arduino-esp32 to the newest release, and avoid this issue altogether..

[atanisoft]

@qt1 arduino-esp32 is using the 4.2 branch of esp-idf on the 4.2 branch of arduino-esp32. Master uses 3.3.

In both cases it uses gnu++11 and not later as that is what esp-idf uses/supports.

But I do agree, it seems hacky. Esp-idf master has moved up to FreeRTOS 10.x and is still in flux as there are many customized pieces for multi-core support

[0xjakob]

Since commit PR #6692 f6031d4 it should be possible to compile with standard C++20 in user projects (in contrast to gnu C++ 20). Hence, closing this issue.