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

upgrade llvm version to 17.0.6 #11301

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

trns1997
Copy link
Contributor

@trns1997 trns1997 commented Dec 1, 2023

Summary

Related to #11269
Upgrade to LLVM version 17.0.6
Also set Nuttx libm as default math lib to prevent compilation failures

Tested with gcc version 12.3.Rel1 and 13.2.rel1

Impact

Testing

@trns1997 trns1997 changed the title upgrade llvm version to 17.0.6 and set default nuttx math lib as default Draft: upgrade llvm version to 17.0.6 and set default nuttx math lib as default Dec 1, 2023
@trns1997 trns1997 changed the title Draft: upgrade llvm version to 17.0.6 and set default nuttx math lib as default upgrade llvm version to 17.0.6 and set default nuttx math lib as default Dec 1, 2023
@trns1997 trns1997 marked this pull request as draft December 1, 2023 14:34
@trns1997 trns1997 force-pushed the upgrade_llvm_version_17.0.6 branch 2 times, most recently from b422879 to 4f7986c Compare December 1, 2023 14:44
@trns1997 trns1997 marked this pull request as ready for review December 1, 2023 14:45
@trns1997 trns1997 force-pushed the upgrade_llvm_version_17.0.6 branch 2 times, most recently from 4d3ef61 to a05ac7f Compare December 1, 2023 15:31
@xiaoxiang781216
Copy link
Contributor

@trns1997 could we keep the default math config(CONFIG_LIBM_TOOLCHAIN)?

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 2, 2023

@trns1997 could we keep the default math config(CONFIG_LIBM_TOOLCHAIN)?

@xiaoxiang781216 we could but then it won't compile unless you use the math library from nuttx. Otherwise we need to figure out a solution for the tool chain math lib with llvm.

@xiaoxiang781216
Copy link
Contributor

what's error report from toolchain's math library?

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 2, 2023

what's error report from toolchain's math library?

I explained the issue over here : #11269 (comment)

If you have any other suggestion i am open to them.

@xiaoxiang781216
Copy link
Contributor

what's error report from toolchain's math library?

I explained the issue over here : #11269 (comment)

If you have any other suggestion i am open to them.

let's remove #if statement:

#if defined(__need_wint_t) || defined(__need_mbstate_t)

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif

#include_next <wchar.h>

@trns1997 trns1997 closed this Dec 4, 2023
@trns1997 trns1997 reopened this Dec 4, 2023
@xiaoxiang781216
Copy link
Contributor

@trns1997 why the ci pass now without wchar.h error?

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 4, 2023

@trns1997 why the ci pass now without wchar.h error?

Ummm I didnt expect the CI to pass. I am guessing it is because we do not have any LIBCXX specific tests or LIBM is always activated when compiling with LIBCXX

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 4, 2023

what's error report from toolchain's math library?

I explained the issue over here : #11269 (comment)
If you have any other suggestion i am open to them.

let's remove #if statement:

#if defined(__need_wint_t) || defined(__need_mbstate_t)

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif

#include_next <wchar.h>

I was thinking of creating a patch file that we apply when compiling. But depends on what we want to do. Should we merge this and keep a note somewhere saying if you face our problem activate the LIBM option?

@xiaoxiang781216
Copy link
Contributor

what's error report from toolchain's math library?

I explained the issue over here : #11269 (comment)
If you have any other suggestion i am open to them.

let's remove #if statement:

#if defined(__need_wint_t) || defined(__need_mbstate_t)

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif

#include_next <wchar.h>

I was thinking of creating a patch file that we apply when compiling. But depends on what we want to do. Should we merge this and keep a note somewhere saying if you face our problem activate the LIBM option?

many defconfig doesn't use the internal math library, here is the list:

boards/arm/imx6/sabre-6quad/configs/libcxx/defconfig
boards/arm/mx8mp/verdin-mx8mp/configs/nsh/defconfig
boards/sim/sim/sim/configs/citest/defconfig
boards/sim/sim/sim/configs/libcxxtest/defconfig

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 4, 2023

what's error report from toolchain's math library?

I explained the issue over here : #11269 (comment)
If you have any other suggestion i am open to them.

let's remove #if statement:

#if defined(__need_wint_t) || defined(__need_mbstate_t)

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif

#include_next <wchar.h>

I was thinking of creating a patch file that we apply when compiling. But depends on what we want to do. Should we merge this and keep a note somewhere saying if you face our problem activate the LIBM option?

many defconfig doesn't use the internal math library, here is the list:

boards/arm/imx6/sabre-6quad/configs/libcxx/defconfig
boards/arm/mx8mp/verdin-mx8mp/configs/nsh/defconfig
boards/sim/sim/sim/configs/citest/defconfig
boards/sim/sim/sim/configs/libcxxtest/defconfig

AAAAAAAHHHHHH wait I know what happening i forgot to push the default LIBCXX version.

@trns1997 trns1997 force-pushed the upgrade_llvm_version_17.0.6 branch 2 times, most recently from b5942b0 to ea9fd5c Compare December 4, 2023 14:39
@xiaoxiang781216
Copy link
Contributor

let's wait ci result. @trns1997 could you send the change to llvm community, so we can remove the patch when llvm release the new package.

@xiaoxiang781216
Copy link
Contributor

both macOS and Linux report __mbstate_t error:

Error: /Applications/Xcode_14.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/_types/_mbstate_t.h:32:28: error: typedef redefinition with different types ('__darwin_mbstate_t' (aka '__mbstate_t') vs 'struct mbstate_s')
typedef __darwin_mbstate_t mbstate_t;
In file included from /github/workspace/sources/nuttx/include/libcxx/__mbstate_t.h:43,
                 from /github/workspace/sources/nuttx/include/libcxx/__std_mbstate_t.h:14,
                 from /github/workspace/sources/nuttx/include/libcxx/iosfwd:106,
                 from /github/workspace/sources/nuttx/include/libcxx/__random/uniform_int_distribution.h:20,
                 from /github/workspace/sources/nuttx/include/libcxx/__algorithm/sample.h:18,
                 from /github/workspace/sources/nuttx/include/libcxx/__algorithm/ranges_sample.h:13,
                 from /github/workspace/sources/nuttx/include/libcxx/algorithm:1893,
                 from libcxx/src/algorithm.cpp:9:
Error: /usr/include/x86_64-linux-gnu/bits/types/mbstate_t.h:6:21: error: conflicting declaration 'typedef struct __mbstate_t mbstate_t'
    6 | typedef __mbstate_t mbstate_t;
      |                     ^~~~~~~~~

We should avoid [_]mbstate_t.h from host get included.

@trns1997
Copy link
Contributor Author

trns1997 commented Dec 5, 2023

both macOS and Linux report __mbstate_t error:

Error: /Applications/Xcode_14.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/_types/_mbstate_t.h:32:28: error: typedef redefinition with different types ('__darwin_mbstate_t' (aka '__mbstate_t') vs 'struct mbstate_s')
typedef __darwin_mbstate_t mbstate_t;
In file included from /github/workspace/sources/nuttx/include/libcxx/__mbstate_t.h:43,
                 from /github/workspace/sources/nuttx/include/libcxx/__std_mbstate_t.h:14,
                 from /github/workspace/sources/nuttx/include/libcxx/iosfwd:106,
                 from /github/workspace/sources/nuttx/include/libcxx/__random/uniform_int_distribution.h:20,
                 from /github/workspace/sources/nuttx/include/libcxx/__algorithm/sample.h:18,
                 from /github/workspace/sources/nuttx/include/libcxx/__algorithm/ranges_sample.h:13,
                 from /github/workspace/sources/nuttx/include/libcxx/algorithm:1893,
                 from libcxx/src/algorithm.cpp:9:
Error: /usr/include/x86_64-linux-gnu/bits/types/mbstate_t.h:6:21: error: conflicting declaration 'typedef struct __mbstate_t mbstate_t'
    6 | typedef __mbstate_t mbstate_t;
      |                     ^~~~~~~~~

We should avoid [_]mbstate_t.h from host get included.

hey @xiaoxiang781216 ok so we have 2 problems.

First problem is the following:

Error: /github/workspace/sources/nuttx/include/libcxx/__iterator/segmented_iterator.h:72:46: error: template argument '(sizeof (_Tp) * 0)' involves template parameter(s)

This i figured out why. Basically i pulled the docker image and found the the gcc version 11 and for newer llvm versions require gcc version >= 12. I updated the gcc version in my local container to test and it resolved this specific issue. So we will need to update the docker image (not really sure how to do it for nuttx).

The second problem that you mention, I looked into the code of libs/libxx/libcxx/include/_mbstate_t.h:

#if defined(_LIBCPP_HAS_MUSL_LIBC)
#   define __NEED_mbstate_t
#   include <bits/alltypes.h>
#   undef __NEED_mbstate_t
#elif __has_include(<bits/types/mbstate_t.h>)
#   include <bits/types/mbstate_t.h> // works on most Unixes
#elif __has_include(<sys/_types/_mbstate_t.h>)
#   include <sys/_types/_mbstate_t.h> // works on Darwin
#elif !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS) && __has_include_next(<wchar.h>)
#   include_next <wchar.h> // fall back to the C standard provider of mbstate_t
#elif __has_include_next(<uchar.h>)
#   include_next <uchar.h> // <uchar.h> is also required to make mbstate_t visible
#else
#   error "We don't know how to get the definition of mbstate_t without <wchar.h> on your platform."
#endif

The unix build will always fail because it is able to find #elif __has_include(<bits/types/mbstate_t.h>) as the files comes with the toolchain of linux. One solution is to check if we're compiling on linux and then we define #define _LIBCPP_HAS_NO_WIDE_CHARACTERS in the libs/libxx/__config_site file but i am not sure it is the right approach as we want to sim the nuttx build using our nuttx apis and not the host systems.

@xiaoxiang781216
Copy link
Contributor

@trns1997 you can rebase your patch again, all ci issue should be fixed.

@xiaoxiang781216
Copy link
Contributor

@trns1997 could you rebase the change again? All ci issues are fixed.

@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review December 12, 2023 17:27
@xiaoxiang781216
Copy link
Contributor

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

Copy link
Contributor

@pkarashchenko pkarashchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@trns1997
Copy link
Contributor Author

@xiaoxiang781216 we the following error:

Error: /github/workspace/sources/apps/netutils/jsoncpp/jsoncpp/include/json/config.h:132:45: error: 'template<class _CharT> struct std::__1::char_traits' is deprecated: char_traits<T> for T not equal to char, wchar_t, char8_t, char16_t or char32_t is non-standard and is provided for a temporary period. It will be removed in LLVM 18, so please migrate off of it. [-Werror=deprecated-declarations]

Can I modify the apps so that we no longer have the deprecated error?

@xuxin930
Copy link
Contributor

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

hi @trns1997 let's try this trns1997#1 with CI

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 we the following error:

Error: /github/workspace/sources/apps/netutils/jsoncpp/jsoncpp/include/json/config.h:132:45: error: 'template<class _CharT> struct std::__1::char_traits' is deprecated: char_traits<T> for T not equal to char, wchar_t, char8_t, char16_t or char32_t is non-standard and is provided for a temporary period. It will be removed in LLVM 18, so please migrate off of it. [-Werror=deprecated-declarations]

Can I modify the apps so that we no longer have the deprecated error?

sure.

@xuxin930
Copy link
Contributor

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

hi @trns1997 let's try this trns1997#1 with CI

hi @xiaoxiang781216
#11395 and this PR depend on each other in CI matter, so both will fail.
I suggest that temporarily turn off the matter in this PR and then turn it on later in other.
or directly submit cmake commit to this branch and run ci together.
what's your suggestion

@xiaoxiang781216 xiaoxiang781216 force-pushed the upgrade_llvm_version_17.0.6 branch 3 times, most recently from be40b69 to 3386c9d Compare December 15, 2023 01:15
@trns1997
Copy link
Contributor Author

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

hi @trns1997 let's try this trns1997#1 with CI

hi @xiaoxiang781216 #11395 and this PR depend on each other in CI matter, so both will fail. I suggest that temporarily turn off the matter in this PR and then turn it on later in other. or directly submit cmake commit to this branch and run ci together. what's your suggestion

Sorry I was out of town. I guess you guys have opted to disable sim:matter for the moment.

@xiaoxiang781216 any idea to why the linker might be unable to find the following:

arm-none-eabi-ld: warning: /home/trns1997/nuttxspace/nuttx/nuttx has a LOAD segment with RWX permissions
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_getc(file_struct*, wchar_t*)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:125: undefined reference to `getwc'
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_ungetc(int, file_struct*, wchar_t)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:140: undefined reference to `ungetwc'

@xiaoxiang781216
Copy link
Contributor

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

hi @trns1997 let's try this trns1997#1 with CI

hi @xiaoxiang781216 #11395 and this PR depend on each other in CI matter, so both will fail. I suggest that temporarily turn off the matter in this PR and then turn it on later in other. or directly submit cmake commit to this branch and run ci together. what's your suggestion

Sorry I was out of town. I guess you guys have opted to disable sim:matter for the moment.

Yes.

@xiaoxiang781216 any idea to why the linker might be unable to find the following:

arm-none-eabi-ld: warning: /home/trns1997/nuttxspace/nuttx/nuttx has a LOAD segment with RWX permissions
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_getc(file_struct*, wchar_t*)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:125: undefined reference to `getwc'
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_ungetc(int, file_struct*, wchar_t)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:140: undefined reference to `ungetwc'

NutttX's libc doesn't implment getwc/ungetwc yet, @extinguish will provide an implementation soon.

@trns1997
Copy link
Contributor Author

@trns1997 I have your patch which could pass ci now. sim:matter is disabled because CMake script need be modified to work with new libcxx. @zhhyu7 and @xuxin930 will fix it tomorrow.

hi @trns1997 let's try this trns1997#1 with CI

hi @xiaoxiang781216 #11395 and this PR depend on each other in CI matter, so both will fail. I suggest that temporarily turn off the matter in this PR and then turn it on later in other. or directly submit cmake commit to this branch and run ci together. what's your suggestion

Sorry I was out of town. I guess you guys have opted to disable sim:matter for the moment.

Yes.

@xiaoxiang781216 any idea to why the linker might be unable to find the following:

arm-none-eabi-ld: warning: /home/trns1997/nuttxspace/nuttx/nuttx has a LOAD segment with RWX permissions
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_getc(file_struct*, wchar_t*)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:125: undefined reference to `getwc'
arm-none-eabi-ld: /home/trns1997/nuttxspace/nuttx/staging/libxx.a(iostream.o): in function `std::__1::__do_ungetc(int, file_struct*, wchar_t)':
/home/trns1997/nuttxspace/nuttx/libs/libxx/libcxx/src/std_stream.h:140: undefined reference to `ungetwc'

NutttX's libc doesn't implment getwc/ungetwc yet, @extinguish will provide an implementation soon.

Alright @xiaoxiang781216 could you ping the PR here, that way once it's merged i can rebase and run the CI again.

@xiaoxiang781216
Copy link
Contributor

Sure.

@xiaoxiang781216
Copy link
Contributor

Here is the pr: #11408

ThomasNS and others added 2 commits December 17, 2023 17:40
@xiaoxiang781216 xiaoxiang781216 merged commit 17458c7 into apache:master Dec 17, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants