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

build: Split off functionality that depends on libunwind #14

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

Previously lib-compiler-rt depended on libunwind as it pulled in <unwind.h>; this dependency however is isolated in gcc_personality_v0.c. This change makes gcc personality an optional part of lib-compiler-rt, allowing it to be built without an entire supporting C++ runtime.

This will allow lib-compiler-rt to be minimally pulled in when the compiler emits calls to builtin functions e.g., as discussed in unikraft/lib-musl#46.

Tested on qemu-kvm-x86_64 with GCC 13 and clang 16.

@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 28, 2023
@razvand razvand added the enhancement New feature or request label Jun 28, 2023
@razvand razvand self-assigned this Jun 28, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

@andreittr I don't know how related to this pr this is, but it seems that compiler-rt relies on musl, as it looks for float.h, so we should add that as a dependency.
Everything else looks good, so if you think we should do that in another PR we can merge this.

@andreittr
Copy link
Contributor Author

@andreittr I don't know how related to this pr this is, but it seems that compiler-rt relies on musl, as it looks for float.h, so we should add that as a dependency. Everything else looks good, so if you think we should do that in another PR we can merge this.

Good catch; looking through the code I think the right course of action is to add in float.h as part of nolibc as opposed to mandating musl (or any other libc). This is because float.h only defines some useful float constants and is usually provided along with other compiler headers, not tied to a particular libc.
I'll add float.h to nolibc and open a separate PR for that.

@andreittr
Copy link
Contributor Author

On closer inspection it seems compiler-rt requires a math library (in addition to some convenience headers) in order to successfully build. Will look into what the best way to proceed, either depend on (or check for) a real libc, or further split off maths functionality, and handle the issue in a separate PR.

@andreittr
Copy link
Contributor Author

Update: the implicit dependency on a real libc has been fixed with these 2 PRs:

As soon as these are merged, this PR should be rebased on top.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thanks.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Member

@eduardvintila eduardvintila left a comment

Choose a reason for hiding this comment

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

Good addition, thanks.

Reviewed-by: Eduard Vintilă eduard.vintila47@gmail.com

Previously lib-compiler-rt depended on libunwind as it pulled in
<unwind.h>; this dependency however is isolated in gcc_personality_v0.c.
This change makes gcc personality an optional part of lib-compiler-rt,
allowing it to be built without an entire supporting C++ runtime.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

5 participants