[BUILD] Fix build for esp32 #3155

albkharisov · 2024-11-20T17:12:15Z

Fix for toolchains that alias int32_t to long int. In this way we don't have int in variant of attributes.
If it's defined as strict int32_t, int64_t etc, I think it's better to have strict types everywhere.
ESP-IDF (v5.3.1) toolchain fails to compile as they don't have int in attributes variant list.

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

Fix for toolchains that alias int32_t as long int and in this way confuse all the honest people.

linux-foundation-easycla · 2024-11-20T17:12:22Z

The committers listed above are authorized under a signed CLA.

✅ login: albkharisov / name: Albert Kharisov (48678de, d160f8a, 444d869)
✅ login: marcalff / name: Marc Alff (75eb177, 9db948d, 851da95)

netlify · 2024-11-20T17:12:29Z

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	`75eb177`
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/674388a817e7a60008130b3b

marcalff

Approved the workflow to run CI, but blocking the merge.

I would like first to understand the issue in more details,
to decide the best course of action for the fix.

albkharisov · 2024-11-23T13:17:23Z

@marcalff
Okay, here are the details:

I've integrated the opentelemetry-cpp library for ESP32 project. In the middle of this process main difficulty was to make it buildable on ESP-IDF (v5.3.1) toolchain.
ESP-IDF compiler is: xtensa-esp-elf-g++ (crosstool-NG esp-13.2.0_20240530) 13.2.0

Repo was configured as follows:

cmake -B build \
    -DCMAKE_TOOLCHAIN_FILE=/opt/esp-idf2/tools/cmake/toolchain-esp32s3.cmake \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_POSITION_INDEPENDENT_CODE=FALSE \
    -DWITH_ZIPKIN=OFF \
    -DBUILD_TESTING=OFF \
    -DWITH_OTLP_GRPC=OFF \
    -DWITH_OTLP_HTTP=OFF \
    -DBUILD_SHARED_LIBS=OFF \
    -DCMAKE_INSTALL_PREFIX=./build/optl \
    -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
    -DWITH_EXAMPLES=OFF \
    -DCMAKE_SYSTEM_PROCESSOR=riscv

CMAKE_SYSTEM_PROCESSOR is here only to pass cmake configure checks, it doesn't affect the OT project. I suppose we have to add here 'xtensa' too.
Toolchain file is common:

include($ENV{IDF_PATH}/tools/cmake/utilities.cmake)

set(CMAKE_SYSTEM_NAME Generic)

set(CMAKE_C_COMPILER xtensa-esp32s3-elf-gcc)
set(CMAKE_CXX_COMPILER xtensa-esp32s3-elf-g++)
set(CMAKE_ASM_COMPILER xtensa-esp32s3-elf-gcc)
set(_CMAKE_TOOLCHAIN_PREFIX xtensa-esp32s3-elf-)

remove_duplicated_flags("-mlongcalls \
                         -fno-builtin-memcpy -fno-builtin-memset -fno-builtin-bzero \
                         -fno-builtin-stpcpy -fno-builtin-strncpy \
                         ${CMAKE_C_FLAGS}" UNIQ_CMAKE_C_FLAGS)
set(CMAKE_C_FLAGS "${UNIQ_CMAKE_C_FLAGS}" CACHE STRING "C Compiler Base Flags" FORCE)
remove_duplicated_flags("-mlongcalls \
                         -fno-builtin-memcpy -fno-builtin-memset -fno-builtin-bzero \
                         -fno-builtin-stpcpy -fno-builtin-strncpy \
                         ${CMAKE_CXX_FLAGS}" UNIQ_CMAKE_CXX_FLAGS)
set(CMAKE_CXX_FLAGS "${UNIQ_CMAKE_CXX_FLAGS}" CACHE STRING "C++ Compiler Base Flags" FORCE)
remove_duplicated_flags("-mlongcalls ${CMAKE_ASM_FLAGS}" UNIQ_CMAKE_ASM_FLAGS)
set(CMAKE_ASM_FLAGS "${UNIQ_CMAKE_ASM_FLAGS}" CACHE STRING "ASM Compiler Base Flags" FORCE)

Result compile command:

/opt/espressif/tools/xtensa-esp-elf/esp-13.2.0_20240530/xtensa-esp-elf/bin/xtensa-esp32s3-elf-g++ \
    -DOPENTELEMETRY_ABI_VERSION_NO=1 \
    -I/home/albert/work/optl/api/include \
    -I/home/albert/work/optl/sdk/include \
    -I/home/albert/work/optl/sdk \
    -I/home/albert/work/optl/ext/include \
    -mlongcalls \
    -fno-builtin-memcpy \
    -fno-builtin-memset \
    -fno-builtin-bzero \
    -fno-builtin-stpcpy \
    -fno-builtin-strncpy \
    -O3 \
    -DNDEBUG \
    -Wno-error=deprecated-declarations \
    -o \
    CMakeFiles/opentelemetry_trace.dir/tracer_context.cc.obj \
    -c \
    /home/albert/work/optl/sdk/src/trace/tracer_context.cc

This code compiles perfect with host compiler (which is GCC 13.2 by the way, and this stressed me a lot how can 2 similar compilers with exact same flags produce different output!).

But with ESP-IDF it produces an error:

In file included from /home/albert/work/optl/sdk/include/opentelemetry/sdk/common/empty_attributes.h:10,
                 from /home/albert/work/optl/sdk/include/opentelemetry/sdk/trace/recordable.h:11,
                 from /home/albert/work/optl/sdk/include/opentelemetry/sdk/trace/multi_recordable.h:10,
                 from /home/albert/work/optl/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h:11,
                 from /home/albert/work/optl/sdk/src/trace/tracer_context.cc:11:
/home/albert/work/optl/api/include/opentelemetry/common/key_value_iterable_view.h: In instantiation of 'bool opentelemetry::v1::common::KeyValueIterableView<T>::ForEachKeyValue(opentelemetry::v1::nostd::function_ref<bool(opentelemetry::v1::nostd::string_view, absl::otel_v1::variant<bool, long int, long long int, long unsigned int, double, const char*, opentelemetry::v1::nostd::string_view, opentelemetry::v1::nostd::span<const bool, 4294967295>, opentelemetry::v1::nostd::span<const long int, 4294967295>, opentelemetry::v1::nostd::span<const long long int, 4294967295>, opentelemetry::v1::nostd::span<const long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const double, 4294967295>, opentelemetry::v1::nostd::span<const opentelemetry::v1::nostd::string_view, 4294967295>, long long unsigned int, opentelemetry::v1::nostd::span<const long long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const unsigned char, 4294967295> >)>) const [with T = std::array<std::pair<std::__cxx11::basic_string<char>, int>, 0>]':
/home/albert/work/optl/api/include/opentelemetry/common/key_value_iterable_view.h:64:8:   required from here
/home/albert/work/optl/api/include/opentelemetry/common/key_value_iterable_view.h:71:20: error: no match for call to '(opentelemetry::v1::nostd::function_ref<bool(opentelemetry::v1::nostd::string_view, absl::otel_v1::variant<bool, long int, long long int, long unsigned int, double, const char*, opentelemetry::v1::nostd::string_view, opentelemetry::v1::nostd::span<const bool, 4294967295>, opentelemetry::v1::nostd::span<const long int, 4294967295>, opentelemetry::v1::nostd::span<const long long int, 4294967295>, opentelemetry::v1::nostd::span<const long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const double, 4294967295>, opentelemetry::v1::nostd::span<const opentelemetry::v1::nostd::string_view, 4294967295>, long long unsigned int, opentelemetry::v1::nostd::span<const long long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const unsigned char, 4294967295> >)>) (const std::__cxx11::basic_string<char>&, const int&)'
   71 |       if (!callback(iter->first, iter->second))
      |            ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/albert/work/optl/api/include/opentelemetry/common/key_value_iterable.h:7,
                 from /home/albert/work/optl/sdk/include/opentelemetry/sdk/common/attribute_utils.h:15,
                 from /home/albert/work/optl/sdk/include/opentelemetry/sdk/resource/resource.h:8,
                 from /home/albert/work/optl/sdk/src/trace/tracer_context.cc:9:
/home/albert/work/optl/api/include/opentelemetry/nostd/function_ref.h:89:5: note: candidate: 'R opentelemetry::v1::nostd::function_ref<R(Args ...)>::operator()(Args ...) const [with R = bool; Args = {opentelemetry::v1::nostd::string_view, absl::otel_v1::variant<bool, long int, long long int, long unsigned int, double, const char*, opentelemetry::v1::nostd::string_view, opentelemetry::v1::nostd::span<const bool, 4294967295>, opentelemetry::v1::nostd::span<const long int, 4294967295>, opentelemetry::v1::nostd::span<const long long int, 4294967295>, opentelemetry::v1::nostd::span<const long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const double, 4294967295>, opentelemetry::v1::nostd::span<const opentelemetry::v1::nostd::string_view, 4294967295>, long long unsigned int, opentelemetry::v1::nostd::span<const long long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const unsigned char, 4294967295> >}]'
   89 |   R operator()(Args... args) const { return invoker_(callable_, std::forward<Args>(args)...); }
      |     ^~~~~~~~
/home/albert/work/optl/api/include/opentelemetry/nostd/function_ref.h:89:20: note:   no known conversion for argument 2 from 'const int' to 'absl::otel_v1::variant<bool, long int, long long int, long unsigned int, double, const char*, opentelemetry::v1::nostd::string_view, opentelemetry::v1::nostd::span<const bool, 4294967295>, opentelemetry::v1::nostd::span<const long int, 4294967295>, opentelemetry::v1::nostd::span<const long long int, 4294967295>, opentelemetry::v1::nostd::span<const long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const double, 4294967295>, opentelemetry::v1::nostd::span<const opentelemetry::v1::nostd::string_view, 4294967295>, long long unsigned int, opentelemetry::v1::nostd::span<const long long unsigned int, 4294967295>, opentelemetry::v1::nostd::span<const unsigned char, 4294967295> >'
   89 |   R operator()(Args... args) const { return invoker_(callable_, std::forward<Args>(args)...); }
      |                ~~~~^~~~~~~~

The cause of this error is that here you have

static const
opentelemetry::common::KeyValueIterableView<
      std::array<std::pair<std::string, int>, 0>>
      kEmptyAttributes(array);

and int here is crucial.

The thing is that here we have a set of allowed values for Attribute:

using AttributeValue =
    nostd::variant<bool,
                   int32_t,
                   int64_t,
                   uint32_t,
                   double,
                   const char *,
                   nostd::string_view,
                   nostd::span<const bool>,
                   nostd::span<const int32_t>,
                   nostd::span<const int64_t>,
                   nostd::span<const uint32_t>,
                   nostd::span<const double>,
                   nostd::span<const nostd::string_view>,
                   // Not currently supported by the specification, but reserved for future use.
                   // Added to provide support for all primitive C++ types.
                   uint64_t,
                   // Not currently supported by the specification, but reserved for future use.
                   // Added to provide support for all primitive C++ types.
                   nostd::span<const uint64_t>,
                   // Not currently supported by the specification, but reserved for future use.
                   // See https://github.com/open-telemetry/opentelemetry-specification/issues/780
                   nostd::span<const uint8_t>>;

And it appeals to int32_t.
If we do g++ -E -P we can see the difference in 2 compilers (host and ESP-IDF):

And it results in failing detection int resolution for variant in AttributeValue.

I think the best solution is to change all random non-defined int/long int/long long int values to strict sized types. And I'm saying this not because I'm related to embedded field, but because you specified strict types for AttributeValue, and specifying non-strict in other part seems inconsistent.

Possible side-effects:
I don't think it could lead to something unspecified because int on PC-systems is unlikely be different from int32_t, but on embedded systems it would be better to specify exact type int32_t as it could lead to more predictability.
The worst what can happen: projects compiled on toolchains with int == int64 could have some side-effects.

P.S.
It's not the last PR for bringing up OT on ESP32. This will only fix compile problems. I also have 2 suggestions about:

possibility to specify NO_OS value for CMake, because there's no AtFork in pthread realization for ESP32, so fork_unix.cc is not suitable.
there is a slight typo in cmake.in: if (OR val OR val)

P.P.S.
I also want to add packages for ESP registry to have easy and seamless integration OT into ESP32 projects.

albkharisov · 2024-11-23T15:03:30Z

I don't know how to put this to already created PR, but this can fix this abandoned issue #2483

marcalff · 2024-11-23T15:57:33Z

Thanks for all the details, looking.

marcalff · 2024-11-23T16:16:17Z

@albkharisov

Could you clarify, what is the value of sizeof(int) for this platform, and what does INT_MAX looks like in header files (limits.h) ?

short int is 16 bits, long int is 32, long long int is 64, according to the screen shot.

Unblock

sdk/include/opentelemetry/sdk/trace/span_data.h

sdk/include/opentelemetry/sdk/common/empty_attributes.h

marcalff · 2024-11-23T17:40:49Z

P.S. It's not the last PR for bringing up OT on ESP32. This will only fix compile problems. I also have 2 suggestions about:
* possibility to specify NO_OS value for CMake, because there's no AtFork in pthread realization for ESP32, so [fork_unix.cc](https://github.com/open-telemetry/opentelemetry-cpp/blob/main/sdk/src/common/platform/fork_unix.cc#L16) is not suitable.

Ok, let's see this in a different PR.

* there is a slight [typo in cmake.in](https://github.com/open-telemetry/opentelemetry-cpp/blob/main/cmake/opentelemetry-cpp-config.cmake.in#L108): if (OR val OR val)

Thanks for reporting it.
Addressed in #3165

P.P.S. I also want to add packages for ESP registry to have easy and seamless integration OT into ESP32 projects.

Great, thanks for that.

albkharisov · 2024-11-24T09:52:09Z

@albkharisov

Could you clarify, what is the value of sizeof(int) for this platform, and what does INT_MAX looks like in header files (limits.h) ?

short int is 16 bits, long int is 32, long long int is 64, according to the screen shot.

@marcalff

I (502) Test: sizeof(short int) : 2
I (503) Test: sizeof(int) : 4
I (503) Test: sizeof(long int) : 4
I (503) Test: sizeof(long long int) : 8
I (504) Test: INT_MAX : 2147483647
I (504) Test: INT_MIN : 2147483648
I (504) Test: UINT_MAX : 4294967295

It's 4 bytes, but it doesn't help since it doesn't have an alias for int32_t, so for ESP-IDF g++ this std::variant looks like set of (long int, long long int, ...), without int.

albkharisov · 2024-11-24T13:54:15Z

It fails to compile test with this.
Error is here: span_data_test.cc

@marcalff
What's best fix?

Fix test
Revert my last commit

UPD:
fixed test

marcalff · 2024-11-24T19:57:14Z

It fails to compile test with this. Error is here: span_data_test.cc

Yes, saw that.

This exposed a different issue: the existing header files are not so clean when it comes to overload resolution, with helpers conflicting with override methods using default values.

@marcalff What's best fix?
* Fix test

* Revert my last commit
UPD: fixed test

Changing the test is only a work around, because the original test code is supposed to work.

I am reverting the change back to your original patch, as it is good enough.

The various overrides need some cleanup, but I prefer to address that in a separate PR, and not delay this one because of unrelated issues.

codecov · 2024-11-24T20:02:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (c1ef416) to head (75eb177).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3155   +/-   ##
=======================================
  Coverage   87.86%   87.86%           
=======================================
  Files         195      195           
  Lines        6151     6151           
=======================================
  Hits         5404     5404           
  Misses        747      747

Files with missing lines	Coverage Δ
...nclude/opentelemetry/sdk/common/empty_attributes.h	`100.00% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/span_data.h	`98.69% <ø> (ø)`

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

sdk/test/trace/span_data_test.cc

sdk/include/opentelemetry/sdk/trace/span_data.h

marcalff

LGTM, thanks for the fix, and all the details for this platform.

[BUILD] Fix build for esp32

48678de

Fix for toolchains that alias int32_t as long int and in this way confuse all the honest people.

albkharisov requested a review from a team as a code owner November 20, 2024 17:12

marcalff previously requested changes Nov 20, 2024

View reviewed changes

albkharisov requested a review from marcalff November 23, 2024 14:58

marcalff reviewed Nov 23, 2024

View reviewed changes

sdk/include/opentelemetry/sdk/trace/span_data.h Show resolved Hide resolved

marcalff reviewed Nov 23, 2024

View reviewed changes

sdk/include/opentelemetry/sdk/common/empty_attributes.h Show resolved Hide resolved

albkharisov requested a review from marcalff November 24, 2024 09:55

Remove unused default params

444d869

albkharisov force-pushed the fix_build_for_m32_platforms branch from 7a0249a to 444d869 Compare November 24, 2024 10:18

Fix broken test

d160f8a

albkharisov force-pushed the fix_build_for_m32_platforms branch from d2cdb50 to d160f8a Compare November 24, 2024 14:34

marcalff reviewed Nov 24, 2024

View reviewed changes

sdk/test/trace/span_data_test.cc Outdated Show resolved Hide resolved

marcalff reviewed Nov 24, 2024

View reviewed changes

sdk/include/opentelemetry/sdk/trace/span_data.h Outdated Show resolved Hide resolved

Apply suggestions from code review

9db948d

marcalff reviewed Nov 24, 2024

View reviewed changes

sdk/include/opentelemetry/sdk/trace/span_data.h Outdated Show resolved Hide resolved

marcalff and others added 2 commits November 24, 2024 21:11

Update sdk/include/opentelemetry/sdk/trace/span_data.h

851da95

Merge branch 'main' into fix_build_for_m32_platforms

75eb177

marcalff approved these changes Nov 24, 2024

View reviewed changes

marcalff merged commit 31956f8 into open-telemetry:main Nov 24, 2024
57 checks passed

BrewTestBot mentioned this pull request Nov 26, 2024

opentelemetry-cpp 1.18.0 Homebrew/homebrew-core#199015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUILD] Fix build for esp32 #3155

[BUILD] Fix build for esp32 #3155

albkharisov commented Nov 20, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 20, 2024 •

edited

Loading

netlify bot commented Nov 20, 2024 •

edited

Loading

marcalff left a comment

albkharisov commented Nov 23, 2024

albkharisov commented Nov 23, 2024

marcalff commented Nov 23, 2024

marcalff commented Nov 23, 2024

marcalff commented Nov 23, 2024

albkharisov commented Nov 24, 2024

albkharisov commented Nov 24, 2024 •

edited

Loading

marcalff commented Nov 24, 2024

codecov bot commented Nov 24, 2024 •

edited

Loading

marcalff left a comment

[BUILD] Fix build for esp32 #3155

[BUILD] Fix build for esp32 #3155

Conversation

albkharisov commented Nov 20, 2024 • edited Loading

Changes

linux-foundation-easycla bot commented Nov 20, 2024 • edited Loading

netlify bot commented Nov 20, 2024 • edited Loading

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

marcalff left a comment

Choose a reason for hiding this comment

albkharisov commented Nov 23, 2024

albkharisov commented Nov 23, 2024

marcalff commented Nov 23, 2024

marcalff commented Nov 23, 2024

marcalff commented Nov 23, 2024

albkharisov commented Nov 24, 2024

albkharisov commented Nov 24, 2024 • edited Loading

marcalff commented Nov 24, 2024

codecov bot commented Nov 24, 2024 • edited Loading

Codecov Report

marcalff left a comment

Choose a reason for hiding this comment

albkharisov commented Nov 20, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 20, 2024 •

edited

Loading

netlify bot commented Nov 20, 2024 •

edited

Loading

albkharisov commented Nov 24, 2024 •

edited

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading