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

sys/cpp_new_delete: always enable the module when C++ is used #20348

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 7, 2024

Contribution description

Newer toolchains appear to expect the presence of __dso_handle, so always enable the module.
The linker will collect unused sections anyway.

Testing procedure

sys/cpp_ctors builds again on Ubuntu 23.10.

On master it would produce

/usr/lib/gcc/arm-none-eabi/12.2.1/../../../arm-none-eabi/bin/ld: /usr/lib/gcc/arm-none-eabi/12.2.1/../../../arm-none-eabi/lib/thumb/v7e-m/nofp/libstdc++_nano.a(eh_globals.o): in function `_GLOBAL__sub_I___cxa_get_globals_fast':
./build_nano/thumb/v7e-m/nofp/libstdc++/libsupc++/../../../../../../src/libstdc++-v3/libsupc++/eh_globals.cc:85: undefined reference to `__dso_handle'
/usr/lib/gcc/arm-none-eabi/12.2.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/tests/sys/cpp_ctors/bin/nucleo-wl55jc/tests_cpp_ctors.elf: hidden symbol `__dso_handle' isn't defined

Issues/PRs references

@github-actions github-actions bot added Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Feb 7, 2024
@benpicco benpicco added Area: C++ Area: C++ wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 7, 2024
@riot-ci
Copy link

riot-ci commented Feb 7, 2024

Murdock results

✔️ PASSED

453a8be sys/cpp_new_delete: always enable the module when C++ is used

Success Failures Total Runtime
10013 0 10015 09m:37s

Artifacts

@maribu maribu enabled auto-merge February 7, 2024 10:54
@maribu maribu added this pull request to the merge queue Feb 7, 2024
@maribu
Copy link
Member

maribu commented Feb 7, 2024

@MrKevinWeiss This might be something to backport. I think it is safe to assume that this doesn't break things, and hence, doesn't invalidate release testing.

@benpicco benpicco added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Feb 7, 2024
@MrKevinWeiss
Copy link
Contributor

OK...

@MrKevinWeiss
Copy link
Contributor

Actually, since this is a fix for a specific distro I think it can land just in the 2024.01-branch and we will include it in future point releases if needed (it takes quite a while to get it in otherwise)...

@benpicco
Copy link
Contributor Author

benpicco commented Feb 7, 2024

It should become relevant when riotdocker switches to Ubuntu 24.04

@maribu
Copy link
Member

maribu commented Feb 7, 2024

For the CI the docker image is tagged. But a user running BUILD_IN_DOCKER=1 may expect the most recent docker image to just work.

Anyway, if it is in the 2024.01-branch, that is already good. I personally recommend going for the branch rather than the tag anyway, as the branch gets relevant fixes backported well after the release. The downside is that no regression tests are performed, unless a point release is made.

Merged via the queue into RIOT-OS:master with commit 4df5306 Feb 7, 2024
29 checks passed
@MrKevinWeiss
Copy link
Contributor

It should become relevant when riotdocker switches to Ubuntu 24.04

won't that be after the next release?

@benpicco benpicco deleted the cpp_new_delete-always branch February 7, 2024 13:16
gdoffe added a commit to cogip/mcu-firmware that referenced this pull request Mar 14, 2024
Presence of __dso_handle signal has been solved in RIOT by PR 20348.
Thus removal of -nostartfiles option is no more necessary.
Also bump C++ version to C++20.

[1] RIOT-OS/RIOT#20348
gdoffe added a commit to cogip/mcu-firmware that referenced this pull request Mar 30, 2024
Presence of __dso_handle signal has been solved in RIOT by PR 20348.
Thus removal of -nostartfiles option is no more necessary.
Also bump C++ version to C++20.

[1] RIOT-OS/RIOT#20348
gdoffe added a commit to cogip/mcu-firmware that referenced this pull request Apr 8, 2024
Presence of __dso_handle signal has been solved in RIOT by PR 20348.
Thus removal of -nostartfiles option is no more necessary.
Also CXXEXFLAGS setup can be done before include of RIOT
Makefile.include

[1] RIOT-OS/RIOT#20348
gdoffe added a commit to cogip/mcu-firmware that referenced this pull request Apr 12, 2024
Presence of __dso_handle signal has been solved in RIOT by PR 20348.
Thus removal of -nostartfiles option is no more necessary.
Also CXXEXFLAGS setup can be done before include of RIOT
Makefile.include

[1] RIOT-OS/RIOT#20348
gdoffe added a commit to cogip/mcu-firmware that referenced this pull request Apr 21, 2024
Presence of __dso_handle signal has been solved in RIOT by PR 20348.
Thus removal of -nostartfiles option is no more necessary.
Also CXXEXFLAGS setup can be done before include of RIOT
Makefile.include

[1] RIOT-OS/RIOT#20348

Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C++ Area: C++ wrapper Area: cpu Area: CPU/MCU ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants