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

lib/lib-wamr: Move to musl and address compiler errors #8

Closed
wants to merge 1 commit into from

Conversation

R0mbertus
Copy link
Contributor

This PR addresses issues #3, #4, #6 and #7. It does this by:

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

Good job, @R0mbertus! 🔝

Please see my comments below.

return BHT_ERROR;

- pthread_mutexattr_settype(&mattr, PTHREAD_MUTEX_RECURSIVE_NP);
+ pthread_mutexattr_settype(&mattr, PTHREAD_MUTEX_RECURSIVE);

Choose a reason for hiding this comment

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

Why do you need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is needed as it would otherwise not compile as PTHREAD_MUTEX_RECURSIVE_NP is not defined anywhere in musl pthread library, but PTHREAD_MUTEX_RECURSIVE is.

include/bh_platform.h Outdated Show resolved Hide resolved
patches/0006-recursive_np-to-recursive.patch Outdated Show resolved Hide resolved
@R0mbertus
Copy link
Contributor Author

R0mbertus commented Jul 15, 2023

Addressed requested changes essentials.h doesn't work yet fixed with #define in the include/bh_platform.h

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

Could you rebase your 2 commits into a single one?

I will approve after that.

@R0mbertus
Copy link
Contributor Author

They've been rebased into one, and the offsetoff does work properly now.

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.

@R0mbertus Please add a Co-authored-by tag for the co-author (along with the present sign-off-bt's).
Also please update the commit message to state the changes that were necesarry in order for libwamr to build (and there is a weird S letter at the end of the commit message, after the sign-off-by tags).

@R0mbertus R0mbertus changed the title lib/libwamr: Move to musl and address compiler errors lib/lib-wamr: Move to musl and address compiler errors Jul 17, 2023
@R0mbertus
Copy link
Contributor Author

Changed the commit message to include a list of changes needed to build lib-wamr, and added a Co-authored-by (and remove the S at the end that snuck in)

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.

Great work @R0mbertus, please wrap your commit message to ~80 characters per line, I'll approve it after that.

Move from the `LIBNEWLIBC` and `LIBPTHREAD_EMBEDDED` to `LIBMUSL`.
Changed needed to build lib-wamr were:
* Using the `uk/essentials.h` header for a definition of `offsetof`
* Adding a new `0006` patch file to change `PTHREAD_MUTEX_RECURSIVE_NP` into
`PTHREAD_MUTEX_RECURSIVE` as the former on isn't defined
* Change the `0005` patch file to use modern working unikraft initrd loading
code so it can load the `.wasm` files

Signed-off-by: Robert Klink <roberthklink@gmail.com>
Signed-off-by: Ricardo Mohamedhoesein <rmohamedhoesein3@gmail.com>
Co-authored-by: Ricardo Mohamedhoesein <rmohamedhoesein3@gmail.com>
@R0mbertus
Copy link
Contributor Author

Commit messages for both this PR and the one for app-wamr should be wrapped to 80 columns now

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.

Looks good. Thanks @R0mbertus!

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

Copy link

@RaduNichita RaduNichita 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 now. Thanks a lot, @R0mbertus!

Reviewed-by: Radu Nichita radunichita99@gmail.com

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.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

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

@razvand razvand mentioned this pull request Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants