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 runtime page_size #17382

Closed

Conversation

RossComputerGuy
Copy link
Contributor

Fixes #16331 and makes it possible to use Zig correctly on Apple M1 with Asahi.

This PR implements the std.mem.getPageSize() function. It also changes how std.mem.page_size fundamentally works. Before, we had hard coded every page size for how the builtin.target value was "set up". Now the default behavior is to check if builtin.target.page_size is set, if it then use it, if not then do what we did before. We also now pass the page size from the compiler at build time to builtin.target.page_size. There are also some changes to pass the page size fully around allowing developers to set per executable page sizes. I've tested this out on my Apple M1 Pro running NixOS Asahi and it works.

Copy link
Contributor

@matu3ba matu3ba left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this, as I am quite inactive due to personal matters.

From my point of view adding the page size to the cross-target should include sanity checks for at least hosted environments, clarify object format of BSD, Mac and Windows (can the page size be annotated and reliably read from) and brief usable tooling how the user can debug incorrect page size in the commit message instead of getting a very annoying SEGFAULT, at worst after the program was running for a while.

lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/zig/CrossTarget.zig Outdated Show resolved Hide resolved
@RossComputerGuy
Copy link
Contributor Author

To help prevent certain issues, I could make 2 base functions. Safe, and unsafe which one will cause a panic or assert if the value isn't good and the other just returns a 0. But in most cases, this should work.

@RossComputerGuy RossComputerGuy force-pushed the fix/linux-page-size branch 3 times, most recently from b967d48 to 9c46ec7 Compare October 4, 2023 00:17
@RossComputerGuy
Copy link
Contributor Author

Looks like CI for aarch64-linux-debug is failing from a timeout? Hopefully this isn't a concern since aarch64-linux-release worked and the debug one said cancelled and all tests passed.

@mlugg
Copy link
Member

mlugg commented Oct 7, 2023

Yeah, that may not be your fault - I'll trigger a re-run for that job.

EDIT: ah, you rebased just after I did anyway.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I have a few requests.

Overall, it's an interesting approach you took, but ultimately does not solve the problem, because if you cross compile for aarch64-linux and then try to run the cross-compiled binary on an OS configured for 64KB pages, you will get the same problem.

Instead, please enhance the standard library to determine the page size at runtime where necessary.

I don't have a convenient way to personally test this, so I will be relying on you to see this PR through to completion 🙏

lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/target.zig Outdated Show resolved Hide resolved
lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
@RossComputerGuy
Copy link
Contributor Author

I added the ability to get the page size for macOS using the MachTask structure. I had to make getPageSize() in it public but this is working.

@RossComputerGuy
Copy link
Contributor Author

I'm adding the ability to "cross compile" page sizes based on the CPU option. Should it be a feature rather than a specific field in the CPU model struct? It would be easier to disable/enable 16k or 8k pages in the zig build options. I can see the benefit of each but there's a limiting factor of having a feature.

lib/std/heap.zig Outdated Show resolved Hide resolved
@RossComputerGuy RossComputerGuy force-pushed the fix/linux-page-size branch 10 times, most recently from 39bc46e to a8cd60e Compare March 18, 2024 05:58
@RossComputerGuy RossComputerGuy force-pushed the fix/linux-page-size branch 6 times, most recently from 2a0c2d4 to 47d004a Compare March 22, 2024 00:00
@andrewrk
Copy link
Member

Please leave this alone now. A zig core team member needs to have a look at it. In summary, it doesn't matter what you do. The moment a zig core team member decides to prioritize this, it'll get done. Until then, it won't.

@RossComputerGuy
Copy link
Contributor Author

Even for resolving conflicts?

@andrewrk
Copy link
Member

Yes, please save your efforts and the CI resources.

@andrewrk
Copy link
Member

@RossComputerGuy I'm going to be straight with you. You have written a lot of bugs in this PR, and you repeatedly show signs of not taking the time to fully understand the systems that you are making changes to. This erodes trust. It means I have to scrutinize every line of code you write, which is time-consuming and makes me want to just do the work myself instead of mentoring this code.

I don't want to review this code for the 10th time and find yet more bugs. I want the code to be correct and mergeable before I look at it.

In fact, having this PR open is worse than nothing, because other contributors who want the runtime page size problem to be solved, sit back and don't do their own contributions because they see yours open. It's a fire burning the oxygen in a spaceship.

I am genuinely sorry to make this comment because I know it is harsh, but ultimately, I need this bug to be fixed and your PR is not the best path forward to accomplish that. Please be more respectful of reviewers' time by spending your own time fully understanding the systems that you are making changes to.

@andrewrk andrewrk closed this Apr 15, 2024
@RossComputerGuy
Copy link
Contributor Author

@andrewrk I understand, I'm sorry if it seems as if I were rude. I'm sorry that we couldn't make this happen. Thank you for taking the time to explain.

@andrewrk
Copy link
Member

It's OK, it's only a matter of being inexperienced, and we (ZSF) have to balance mentorship with delivering a product.

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.

Zig fails to compile zig init-exe program on linux-aarch64
9 participants