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.

Sign up for GitHub

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

Jump to bottom

include: Fix macros in order to facilitate running apps on AArch64 #21

Closed
wants to merge 2 commits into from
Closed

include: Fix macros in order to facilitate running apps on AArch64 #21

wants to merge 2 commits into from

Conversation

mariasfiraiala
Copy link
Contributor

@mariasfiraiala mariasfiraiala commented Aug 3, 2022 •
edited
Loading

This PR fixes multiple issues regarding newlib support on AArch64.

Signed-off-by: Maria Sfiraiala [email protected]
Signed-off-by: Eduard Vintilă [email protected]

This was linked to issues Aug 3, 2022
include/limits.h: wrong size for LONG_MAX causes crash on AArch64 apps #19
Closed
include/fcntl.h: mismatch between O_NONBLOCK and FNONBLOCK causes a blocking socket for redis on AArch64 #20
Closed
@mariasfiraiala mariasfiraiala changed the title /include: Fix macros in order to facilitate running apps on AArch64 include: Fix macros in order to facilitate running apps on AArch64 Aug 3, 2022
@razvand razvand self-assigned this Aug 4, 2022
razvand
razvand requested changes Aug 4, 2022
View reviewed changes
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.

Hi, @mariasfiraiala , nice work figuring out the issues. See my comments.

include/fcntl.h Outdated Show resolved Hide resolved
@mariasfiraiala mariasfiraiala force-pushed the fix-aarch64 branch 2 times, most recently from 6a40566 to 8aaa74d Compare August 4, 2022 16:54
@mogasergiu
Copy link
Member

Can confirm this fixes the issue for me as well.

@razvand razvand added this to the v0.11.0 (Janus) milestone Aug 20, 2022
@razvand razvand added bug Something isn't working enhancement New feature or request labels Aug 20, 2022
@mariasfiraiala
Fix size for long types on aarch64
0ced7a8 
Default size of the data model for AArch64 is 8 (64 bits) for long
types, however newlibc ends up defining LONG_MAX as 32 bits.

This issue causes extremely optimized functions, such as memchr (which
parses DTB for apps like SQLite or redis), to crash when compiled for
AArch64.

This commit redefines the size for long types as 64 bits on AArch64.

Signed-off-by: Maria Sfiraiala <[email protected]>
@razvand razvand self-requested a review October 19, 2022 15:25
mogasergiu
mogasergiu reviewed Oct 20, 2022
View reviewed changes
Copy link
Member

@mogasergiu mogasergiu left a comment

Choose a reason for hiding this comment

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

Might be worth adding a Fixes tag here:
Fixes: 58f9f3a3f7b5 ("include/fcntl.h: Replace O_NONBLOCK with new one on arm")

@mariasfiraiala
Copy link
Contributor Author

Might be worth adding a Fixes tag here: Fixes: 58f9f3a3f7b5 ("include/fcntl.h: Replace O_NONBLOCK with new one on arm")

Great suggestion, will do!

mogasergiu
mogasergiu approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@mogasergiu mogasergiu 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: Sergiu Moga [email protected]

@mariasfiraiala
Define O_DIRECTORY with different value
534cea5 
Previous fix regarding confusion between O_NONBLOCK and O_DIRECTORY
values changed O_NONBLOCK to a different number, therefore it drifted
away from FNONBLOCK (for which it was an alias) and caused a blocking
socket.

This commit simply changes the value of O_DIRECTORY, without being
necessary to redefine O_NONBLOCK or FNONBLOCK.

Fixes: 58f9f3a ("include/fcntl.h: Replace O_NONBLOCK with new one
on arm")

Signed-off-by: Maria Sfiraiala <[email protected]>
@mariasfiraiala
Copy link
Contributor Author

mariasfiraiala commented Oct 22, 2022 •
edited
Loading

@skuenzer I've found a value for O_DIRECTORY that should fit our purpose: 0400000000 = 0x4000000. I've checked and has only one bit set, fits in int (used for the open function) and wasn't already used in _default_fcntl.h or fcntl.h.

Hope you find it satisfactory as well.

This was referenced Oct 22, 2022
Closed
Merged
mogasergiu
mogasergiu approved these changes Nov 1, 2022
View reviewed changes
Copy link
Member

@mogasergiu mogasergiu left a comment

Choose a reason for hiding this comment

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

There don't seem to be any other conflicts, I did not pay that much attention last time - my apologies. I checked with some printf's inside sys_open()'s code and the flag seems to be correctly set and execution does land where I expected it to.

Reviewed-by: Sergiu Moga [email protected]

skuenzer
skuenzer approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your investigation. The changes look good!

Reviewed-by: Simon Kuenzer [email protected]

razvand
razvand approved these changes Nov 4, 2022
View reviewed changes
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, @mariasfiraiala

Reviewed-by: Razvan Deaconescu [email protected]
Approved-by: Razvan Deaconescu [email protected]

unikraft-bot pushed a commit that referenced this pull request Nov 16, 2022
@mariasfiraiala @unikraft-bot
Define O_DIRECTORY with different value
dea6ac3 
Previous fix regarding confusion between O_NONBLOCK and O_DIRECTORY
values changed O_NONBLOCK to a different number, therefore it drifted
away from FNONBLOCK (for which it was an alias) and caused a blocking
socket.

This commit simply changes the value of O_DIRECTORY, without being
necessary to redefine O_NONBLOCK or FNONBLOCK.

Fixes: 58f9f3a ("include/fcntl.h: Replace O_NONBLOCK with new one
on arm")

Signed-off-by: Maria Sfiraiala <[email protected]>
Reviewed-by: Sergiu Moga <[email protected]>
Reviewed-by: Simon Kuenzer <[email protected]>
Reviewed-by: Razvan Deaconescu <[email protected]>
Approved-by: Razvan Deaconescu <[email protected]>
Tested-by: Unikraft CI <[email protected]>
GitHub-Closes: #21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mogasergiu mogasergiu mogasergiu approved these changes

@skuenzer skuenzer skuenzer approved these changes

@razvand razvand razvand approved these changes

Assignees

@razvand razvand

Labels
bug Something isn't working ci/merged enhancement New feature or request
Projects
Unikraft Core Roadmap
Status: ✅ Done!
Milestone
v0.11.0 (Janus)
5 participants
@mariasfiraiala @mogasergiu @razvand @skuenzer @unikraft-bot