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/nolibc: Fix & improve headers #1002

Closed
wants to merge 3 commits into from

Conversation

andreittr
Copy link
Contributor

Description of changes

This PR addresses a couple of shortcomings in the headers provided with nolibc:

  • Fix typo of CHAR_BIT being spelled as CHAR_BITS
  • Add missing integer literal macros to stdint.h
  • Add missing floating point constant headers (float.h + dependencies)

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

@andreittr andreittr requested a review from a team as a code owner July 25, 2023 14:53
@razvand razvand self-assigned this Jul 25, 2023
@razvand razvand removed the request for review from a team July 25, 2023 18:03
@razvand razvand added area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix lib/nolibc Only neccessary subset of libc functionality lang/c Issues or PRs to do with C/C++ labels Jul 25, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jul 25, 2023
@DeliaPavel
Copy link
Contributor

Looks good to me! It seems like the checkpatch failed, but I am ready to approve once it passes.

@andreittr
Copy link
Contributor Author

Looks good to me! It seems like the checkpatch failed, but I am ready to approve once it passes.

I believe it's a case of a false positive from a heuristic script: the values of the macros aren't "complex", rather very plain float literals in scientific notation. The '+' must be confusing the script's regexes, but it won't confuse the compiler's lexer.
Regardless, in cases where we import files as-is from upstream projects as opposed to writing new code (like here from musl) I think avoiding unnecessary changes is worthwhile.

Copy link
Contributor

@DeliaPavel DeliaPavel 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: Delia Pavel delia_maria.pavel@stud.acs.upb.ro

@razvand razvand requested a review from StefanJum August 5, 2023 05:43
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.

This looks fine @andreittr, please rebase it on top of staging so the ci/cd tests can run.

This change fixes a typo in the `CHAR_BIT` constant in limits.h.
Previously it was incorrectly spelled as `CHAR_BITS`.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change adds the missing `*_C` integer literal macros to stdint.h.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change adds float.h, along with arch-specific sub-headers, which
define constants regarding floating-point numbers.

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

Checkpatch failed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered errors:

SHA commit checkpatch
c1ca385 lib/nolibc: Fix typo in CHAR_BIT in limits.h
3e59d9d lib/nolibc: Add integer literal macros to stdint.h
c8da4c5 lib/nolibc: Add missing floating point headers

Truncated logs starting from first error c8da4c5:

ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#42: FILE: lib/nolibc/musl-imported/arch/aarch64/bits/float.h:5:
+#define LDBL_MAX 1.18973149535723176508575932662800702e+4932L

ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#68: FILE: lib/nolibc/musl-imported/arch/x86_64/bits/float.h:9:
+#define LDBL_MAX     1.1897314953572317650e+4932L

ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#100: FILE: lib/nolibc/musl-imported/include/float.h:15:
+#define FLT_MAX 3.40282346638528859812e+38F

ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#115: FILE: lib/nolibc/musl-imported/include/float.h:30:
+#define DBL_MAX 1.79769313486231570815e+308

total: 4 errors, 0 warnings, 95 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0003-lib-nolibc-Add-missing-floating-point-headers.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

@razvand razvand requested a review from StefanJum August 7, 2023 15:40
@razvand
Copy link
Contributor

razvand commented Aug 7, 2023

@StefanJum, see this. The checkpatch items can be ignored, they're basically issues with the way checkpatch works. After you approve it, I'll cast the final approve.

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 Aug 7, 2023
This change adds the missing `*_C` integer literal macros to stdint.h.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Delia Pavel <delia_maria.pavel@stud.acs.upb.ro>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #1002
unikraft-bot pushed a commit that referenced this pull request Aug 7, 2023
This change adds float.h, along with arch-specific sub-headers, which
define constants regarding floating-point numbers.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Delia Pavel <delia_maria.pavel@stud.acs.upb.ro>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #1002
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 7, 2023
@andreittr andreittr deleted the ttr/nolibc-headers branch August 7, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI kind/quick-fix Issue is a quick fix lang/c Issues or PRs to do with C/C++ lib/nolibc Only neccessary subset of libc functionality
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants