Skip to content
This repository has been archived by the owner on Mar 2, 2024. It is now read-only.

Change newlib with musl #9

Closed
wants to merge 2 commits into from
Closed

Conversation

JADarius
Copy link
Contributor

This PR changes the the newlib dependency with musl.
In theory, this should work, but when I try to run it and give it any lua instructions it crashes.
Any suggestion would be appreciated.

@JADarius
Copy link
Contributor Author

The error is:

Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
           Atlas 0.13.1~e467cdf6-custom
[    0.166974] Info: [libukboot] <boot.c @  369> Pre-init table at 0x2540b0 - 0x2540b0
[    0.172812] Info: [libukboot] <boot.c @  380> Constructor table at 0x2540b0 - 0x2540b0
[    0.178848] Info: [libukboot] <boot.c @  393> Environment variables:
[    0.183509] Info: [libukboot] <boot.c @  395>        PATH=/bin
[    0.188111] Info: [libukboot] <boot.c @  401> Calling main(1, ['build/app-lua_qemu-x86_64'])
Lua 5.4.4  Copyright (C) 1994-2022 Lua.org, PUC-Rio
> ls
[    2.101800] CRIT: [libkvmplat] <traps.c @   84> Unhandled Trap 6 (invalid opcode), error code=0x0
[    2.108398] Info: [libkvmplat] <traps.c @   86> Regs address 0x28ff50
[    2.113015] CRIT: [libkvmplat] <trace.c @   41> RIP: 0000000000000003 CS: 0008
[    2.118252] CRIT: [libkvmplat] <trace.c @   42> RSP: 00000000002b8968 SS: 0010 EFLAGS: 00010283
[    2.124875] CRIT: [libkvmplat] <trace.c @   44> RAX: 0000000000000000 RBX: 0000000000000228 RCX: 0000000000000000
[    2.131821] CRIT: [libkvmplat] <trace.c @   46> RDX: 0000000000000000 RSI: 00000000002b8930 RDI: 0000000000000000
[    2.138914] CRIT: [libkvmplat] <trace.c @   48> RBP: 0000000000000000 R08: 00000000001f4336 R09: 000000007fffffff
[    2.146161] CRIT: [libkvmplat] <trace.c @   50> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[    2.152967] CRIT: [libkvmplat] <trace.c @   52> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[    2.159765] CRIT: [libkvmplat] <traps.c @   90> Crashing
[    2.164422] Info: [libkvmplat] <shutdown.c @   35> Unikraft halted

@JADarius
Copy link
Contributor Author

From testing with gdb, it seems the issue is related with signals. The function docall from lua.c is the function where the error appears (lua.c:161).

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.

Great job, @JADarius. You are correct - the runtime error is indeed caused by a uksignal issue. There is a PR already on Unikraft core which fixes the problem.

Please sign-off your commit and use present tense in the commit title (i.e. Update newlib references to musl), as per the contributing guidelines.

@JADarius JADarius changed the title Changed newlib with musl Change newlib with musl Jul 15, 2023
@JADarius JADarius force-pushed the staging branch 2 times, most recently from 18b6392 to 554c938 Compare July 15, 2023 14:17
@eduardvintila eduardvintila self-requested a review July 15, 2023 14:42
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 great, @JADarius! Just use "Change" (with a capital "C) instead of "change" in the commit name and I'll approve the PR 👍🏻

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.

All good, thank you!

Reviewed-by: Eduard Vintilă eduard.vintila47@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.

Thanks @JADarius. See the comments and please update the commit message, since this pr will do more than cahnge newlib to musl (it also update the kraft.yaml file and adds the Makefile* files). You can also split the commit into more if you think that's more fit.

kraft.yaml Outdated Show resolved Hide resolved
kraft.yaml Show resolved Hide resolved
@JADarius JADarius requested a review from StefanJum July 18, 2023 06:04
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.

Thanks, @JADarius. I tested and it works.

Please use a prefix such as Makefile*: and kraft.yaml: for the two commits.

Also add an extensive commit description (separate by a blank line from the commit message). The description should provide the motivation and outcome of the commit changes.

This commit changes the newlib dependency with the newer musl one
because it is the new standard.

Signed-off-by: Darius-Andrei Jipa <jipad16@yahoo.com>
Change all the versions to "stable" for better compatibility.

Signed-off-by: Darius-Andrei Jipa <jipad16@yahoo.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.

All good, thanks.
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

unikraft-bot pushed a commit that referenced this pull request Jul 26, 2023
Change all the versions to "stable" for better compatibility.

Signed-off-by: Darius-Andrei Jipa <jipad16@yahoo.com>
Reviewed-by: Eduard Vintilă <eduard.vintila47@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants