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

Add alloc support #278

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Add alloc support #278

merged 1 commit into from
Jan 12, 2024

Conversation

duanyu-yu
Copy link
Contributor

Added alloc support for loader, based on the previous bootstrap and bump allocator implementations from the kernel.

@duanyu-yu duanyu-yu mentioned this pull request Nov 30, 2023
@mkroening mkroening self-requested a review November 30, 2023 18:39
Copy link
Member

@mkroening mkroening 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, Yu! :)

Overall, this looks very promising. I left a few comments with the hope to avoid a few dependencies as well as nightly.

src/main.rs Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
src/allocator/bootstrap.rs Outdated Show resolved Hide resolved
src/allocator/mod.rs Outdated Show resolved Hide resolved
@mkroening mkroening self-assigned this Nov 30, 2023
@mkroening mkroening self-requested a review January 10, 2024 08:13
Copy link
Member

@mkroening mkroening 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!

Code-wise, this looks excellent. Just a few more requests on the form:

  • The Cargo.lock file should not be added to .gitignore. Instead, it should track the minimum changes necessary by this PR.
  • This PR should not include any merge commits. In this case, we might even squash everything into one commit.
  • The formatting CI check fails (run cargo fmt) to fix any formatting issues.

If you need help with rebasing, reach out. :)

@mkroening mkroening self-requested a review January 11, 2024 09:39
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Ah, CI shows that this does not work on UEFI, which makes sense, since UEFI already provides a global allocator.

Let's move the dependencies in Cargo.toml to [target.'cfg(target_os = "none")'.dependencies] and mark mod allocator; with #[cfg(target_os = "none")]. We could move the declaration of the global allocator into the allocator module, too.

Removed nightly features; Simplified allocator

Fixed formatting issues

Move the global allocator into the allocator module
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me! :)

@mkroening mkroening added this pull request to the merge queue Jan 12, 2024
Merged via the queue into hermit-os:main with commit 10152b8 Jan 12, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants