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

cmake: c++ exceptions linking support #35647

Merged

Conversation

tejlmand
Copy link
Collaborator

@tejlmand tejlmand commented May 25, 2021

Fixes part of: #32448

This commit updates the CMake CMAKE_CXX_LINK_EXECUTABLE to include
crtbegin.o and crtend.o at the right locations when linking with gcc.

It also updates linker scripts to ensure proper location of the
exception header frame sections.

Signed-off-by: Kumar Gala kumar.gala@linaro.org
Signed-off-by: Torsten Rasmussen Torsten.Rasmussen@nordicsemi.no


Update:
The following platforms are fixed with this PR:
qemu_x86
qemu_riscv32
qemu_xtensa

The following platforms still fails:
qemu_x86_64 (fails with exit)
qemu_cortex_a53 (fails with exit)
qemu_riscv64 (fails with exit)
qemu_nios2 (hangs)
qemu_leon3 (fails with trap_instruction)

@tejlmand tejlmand requested review from nashif, carlescufi and galak May 25, 2021 21:12
@github-actions github-actions bot added area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Build System area: RISCv32/64 area: X86 x86 Architecture (32-bit) labels May 25, 2021
@tejlmand
Copy link
Collaborator Author

@galak I included your linker script modifications from this: #34357

@tejlmand tejlmand force-pushed the issues/32448_cpp_exception_linking branch from 6292658 to a8e5874 Compare May 25, 2021 21:14
@tejlmand tejlmand added the bug The issue is a bug, or the PR is fixing a bug label May 25, 2021
@tejlmand tejlmand added this to the v2.6.0 milestone May 25, 2021
@tejlmand tejlmand requested review from carlescufi and removed request for carlescufi May 25, 2021 21:15
@dcpleung
Copy link
Member

Could you refactor the linker script addition into a file, so it can be done via #include?

@carlescufi carlescufi requested a review from stephanosio May 26, 2021 07:22
@tejlmand tejlmand force-pushed the issues/32448_cpp_exception_linking branch from a8e5874 to 4b2d0cd Compare May 26, 2021 08:00
@tejlmand tejlmand requested a review from andyross as a code owner May 26, 2021 08:00
@tejlmand
Copy link
Collaborator Author

Could you refactor the linker script addition into a file, so it can be done via #include?

Done, placed it in cplusplus-ram.ld where we already have linker code related to exception tables.

@tejlmand tejlmand force-pushed the issues/32448_cpp_exception_linking branch from 4b2d0cd to e2501fd Compare May 26, 2021 08:09
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

This claims to fix #32448, but there are some platforms that still fail.

Just noting that #32448 should stay open to keep track of them ... (see #32448 (comment)).

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

The following should be updated to test the platforms that work after the changes in this PR:

cpp.libcxx.exceptions:
# FIXME: change when issue is resolved #32448
platform_allow: mps2_an385
toolchain_exclude: xcc
min_flash: 54
tags: cpp
timeout: 5
extra_configs:
- CONFIG_EXCEPTIONS=y

@zephyrbot zephyrbot added the area: RISCV RISCV Architecture (32-bit & 64-bit) label May 26, 2021
@tejlmand
Copy link
Collaborator Author

tejlmand commented May 26, 2021

This claims to fix #32448, but there are some platforms that still fail.

Looking into this, the CPP_TEST provided in #32448 passes on the platforms: qemu_xtensa, qemu_riscv32, qemu_cortex_m3 and qemu_x86 works.
As example:

$ west build -b qemu_xtensa --pristine -t run
-- west build: making build dir /projects/github/ncs/zephyr/tests/CPP_TEST/build pristine
...
*** Booting Zephyr OS build zephyr-v2.5.0-4098-ge2501fd08ef3  ***
Running test suite framework_tests
===================================================================
START - test_assert
 PASS - test_assert in 0.1 seconds
===================================================================
START - test_cexception
 PASS - test_cexception in 0.2 seconds
===================================================================
Test suite framework_tests succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

also some archs passes on tests/subsys/cpp/libcxx when built and executed manually, like qemu_xtensa:

$  west build -b qemu_xtensa --pristine -t run -- -DCONFIG_EXCEPTIONS=y
-- west build: making build dir /projects/github/ncs/zephyr/tests/subsys/cpp/libcxx/build pristine
...
*** Booting Zephyr OS build zephyr-v2.5.0-4098-ge2501fd08ef3  ***
version 201703
Running test suite libcxx_tests
===================================================================
START - test_array
 PASS - test_array in 0.1 seconds
===================================================================
START - test_vector
 PASS - test_vector in 0.1 seconds
===================================================================
START - test_make_unique
 PASS - test_make_unique in 0.1 seconds
===================================================================
START - test_exception
 PASS - test_exception in 0.8 seconds
===================================================================
Test suite libcxx_tests succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL
qemu-system-xtensa: terminating on signal 2

but fails when using twister:

$ ./scripts/twister -v -p qemu_xtensa -N  --inline-logs -s  tests/subsys/cpp/libcxx/cpp.libcxx.exceptions
...
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

INFO    - /projects/github/ncs/zephyr/twister-out/qemu_xtensa/tests/subsys/cpp/libcxx/cpp.libcxx.exceptions/build.log

INFO    - 0 of 1 test configurations passed (0.00%), 1 failed, 0 skipped with 0 warnings in 5.36 seconds
INFO    - In total 4 test cases were executed, 0 skipped on 1 out of total 372 platforms (0.27%)
INFO    - 0 test configurations executed on platforms, 1 test configurations were only built.

other archs (riscv32) fails on tests/subsys/cpp/libcxx but not CPP_TEST.

Marked DNM to allow further investigations.

@tejlmand tejlmand added the DNM This PR should not be merged (Do Not Merge) label May 26, 2021
# are linked at specific locations.
# The location is so important that we cannot let this be controlled by normal
# link libraries, instead we must control the link command specifically as
# part of toolchain.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW: trying to do this on the command line is fragile. Really this needs to happen in a linker script. Also I wouldn't be surprised if the rules are platform-dependent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For GCC this has normally be done via a spec file. @andyross Have you seen anywhere that this is done via linker script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how will you tell the linker which objects (crtbegin.o and crtend.o) to link in a linker file ?
Not to mention that the position of those objects on the linker invocation is important.

This is why this is to be considered a toolchain (linker) setting.

@tejlmand tejlmand force-pushed the issues/32448_cpp_exception_linking branch from e2501fd to 8d096ec Compare May 27, 2021 09:33
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label May 27, 2021
@tejlmand
Copy link
Collaborator Author

Updated description text with list of platform that are fixed with this PR, as it stands.
The following platforms are fixed with this PR:
qemu_x86
qemu_riscv32
qemu_xtensa

The following platforms still fails:
qemu_x86_64 (fails with exit)
qemu_cortex_a53 (fails with exit)
qemu_riscv64 (fails with exit)
qemu_nios2 (hangs)
qemu_leon3 (fails with trap_instruction)

tejlmand added 2 commits May 27, 2021 11:42
Fixes part of: zephyrproject-rtos#32448

This commit updates the CMake CMAKE_CXX_LINK_EXECUTABLE to include
crtbegin.o and crtend.o at the right locations when linking with gcc.

It also updates linker scripts to ensure proper location of the
exception header frame sections.

This ensure proper handling of exceptions for those architectures
- x86
- xtensa
- riscv32

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Increasing ZTEST_STACKSIZE to 4096.
This ensures that the riscv32 platforms can succesfully execute the
C++ exception test cases.

Also add the following platforms to allow list:
- qemu_arc_em
- qemu_arc_hs
- qemu_cortex_m0
- qemu_cortex_m3
- qemu_cortex_r5
- qemu_riscv32
- qemu_x86
- qemu_xtensa

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@tejlmand tejlmand force-pushed the issues/32448_cpp_exception_linking branch from 8d096ec to 33818f0 Compare May 27, 2021 09:44
@tejlmand
Copy link
Collaborator Author

The following should be updated to test the platforms that work after the changes in this PR:

Done.

@tejlmand tejlmand removed the DNM This PR should not be merged (Do Not Merge) label May 27, 2021
@galak galak requested a review from stephanosio May 27, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Build System area: RISCV RISCV Architecture (32-bit & 64-bit) area: Tests Issues related to a particular existing or missing test area: X86 x86 Architecture (32-bit) bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants