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

Added (sub-)sections .gcc_except_table to linker scripts #19002

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

vanwinkeljan
Copy link
Member

Include .gcc_except_table section and it sub-sections in the linker files of supported architectures to allow enabling C++ with exception support.
If these sections are not mapped warnings will be generated for orphaned sections at link time.

Copy link
Contributor

@wentongwu wentongwu left a comment

Choose a reason for hiding this comment

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

Can this be used in user mode of a thread if enabled user space?

Copy link
Contributor

@nategraff-sifive nategraff-sifive left a comment

Choose a reason for hiding this comment

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

RISC-V LGTM

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Regarding userspace: as long as the access to these tables is only read-only (which I assume is true but didn't actually check) there's no reason we can't link it into the same region as the .text segments and share them across userspace apps. But I'd be surprised if it worked out of the box, surely something would have to be fixed.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Regarding ARM: It is good that the sections are at the end, so they won't interfere with static MPU regions, defining in the beginning of the SRAM

@wentongwu wentongwu self-requested a review September 10, 2019 10:30
Copy link
Contributor

@wentongwu wentongwu left a comment

Choose a reason for hiding this comment

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

If there is only read-only stuff, please cleanup read-write segments. But if it needs rw section as current patch indicated, exceptions will not work in user mode.

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I think I understand now, but it took some digging and some of it is theory only.

For some targets binutils will put exception-related data in .gcc_except_table.* as well as .gcc_except_table. So the generic linker solution has to add those, which were absent in xtensa. (ARM thumb apparently doesn't need it.)

The standard binutils scripts also support separating read-only and read-write data. By inspection this PR does that as well. To @wentongwu's point: if that means exceptions won't work in usermode, then they won't work in usermode. When exception support is added the test below fails to link on Nordic with a DW_AT_data_member_location failure, so there's definitely something that needs to be done.

There should be a test for exception handling, not just building with CONFIG_EXCEPTIONS enabled. On ARM adding the test below passes even without the linker script changes.

Several things need to be explained and tested before this should be merged:

  • Why does the augmented test with exceptions enabled pass on ARM without modifying any linker scripts? The in-tree scripts for ARM don't have .gcc_except_table.
  • We need to determine the behavior of CONFIG_EXCEPTIONS combined with user mode, and either make sure it works or document that it won't.
  • We need to test exception functionality on boards other than ARM Cortex-M.
static void test_exceptions(void)
{
	if (!IS_ENABLED(CONFIG_EXCEPTIONS)) {
		TC_PRINT("Feature not enabled\n");
		return;
	}

	try {
		throw std::exception();
		zassert_unreachable("Passed throw");
	} catch (const std::exception& e) {
		TC_PRINT("Caught\n");
	}
}

@vanwinkeljan
Copy link
Member Author

@pabigot

Why does the augmented test with exceptions enabled pass on ARM without modifying any linker scripts? The in-tree scripts for ARM don't have .gcc_except_table.

For ARM it it is put in .ARM.extab, so it could be that .gcc_except_table is not needed for ARM but I could not find much info on it so added it for completeness.

We need to determine the behavior of CONFIG_EXCEPTIONS combined with user mode, and either make sure it works or document that it won't.

Note that this PR is not only fixing a build issue with CONFIG_EXCEPTIONS enabled, exception handling code could also be pulled in by just using libc++. So I would suggest to disable C++ support for user mode until we have something stable/working without user mode and then move on to tackle user mode.

We need to test exception functionality on boards other than ARM Cortex-M.

Agree, did a quick test before and in my case the catch is not working for risc-v, x86 on qemu and results in a terminate. Still need to investigate this issue further, but at least this PR makes it possible to build with CONFIG_EXCEPTIONS enabled or in case exception handling is pulled in from libc++ (eg. #18464).

@pabigot pabigot self-requested a review September 13, 2019 14:10
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I've now had a chance to look at this, and I think it should go in.

While testing I've rebased and extended this; with the changes in this branch (which includes #19142) exception handling in C++ with usermode pass on 11 of 17 tested platforms, including real hardware (nrf52840_pca10056 and frdm_k64f).

@wentongwu Whether exceptions work in userspace depend on whether the target requires exception state in RAM. The linker script modifications in this PR appear to do the right thing; if it fails it's because Zephyr doesn't handle it. Please revisit your review and if you agree remove your "changes requested".

When we do have exceptions and userspace as far as they can be made to work we need to document the conditions and platforms where they will not work. But I don't think that's something this PR needs to address.

@vanwinkeljan Please rebase this since some of the xtensa material has been removed, and remove the empty lines at the ends of include/linker/cplusplus-*.ld: these cause git to complain about whitespace errors. (This has been done in the branch I reference above, so you can just fetch and use that if you want.)

Unless something else shows up I'll approve once the rebase is in place and CI passes.

@vanwinkeljan
Copy link
Member Author

Updated libcxx testcase.yaml to exclude new target pico_pi_m4 as the ROM is to small to support exception handling

@pabigot pabigot self-requested a review September 16, 2019 12:52
Added test run with exceptions enabled.

Boards colibri_imx7d_m4, warp7_m4 and pico_pi_m4 have been excluded
from test run as they do not have sufficient ROM.

Signed-off-by: Jan Van Winkel <[email protected]>
Make sure that all sub-sections of .gcc_except_table are mapped in
rodata else C++ builds with exceptions enabled will generate warnings
due to orphan sections.

Signed-off-by: Jan Van Winkel <[email protected]>
Include .gcc_except_table (sub-)sections in linker files to support C++
with exceptions enabled. If these sections are not mapped warnings will
be generated for orphaned sections at link time.

Signed-off-by: Jan Van Winkel <[email protected]>
@galak galak merged commit ff36fc7 into zephyrproject-rtos:master Sep 19, 2019
@vanwinkeljan vanwinkeljan deleted the fix_cpp_exceptions branch September 19, 2019 15:09
@pabigot pabigot mentioned this pull request Nov 15, 2019
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: C++ area: Linker Scripts area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants