-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add runtime page_size #17382
Conversation
824f901
to
c62a394
Compare
c62a394
to
d78b3c6
Compare
There was a problem hiding this 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.
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. |
b967d48
to
9c46ec7
Compare
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. |
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. |
9c46ec7
to
a282ede
Compare
a282ede
to
0242d33
Compare
There was a problem hiding this 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 🙏
0242d33
to
b7d56b4
Compare
314d650
to
ca1526f
Compare
ca1526f
to
e897307
Compare
e897307
to
31a5e22
Compare
I added the ability to get the page size for macOS using the |
576a2d3
to
28e4a20
Compare
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 |
7e0e107
to
0b956ff
Compare
3a5a7f0
to
714fd4f
Compare
4997c4b
to
53eff28
Compare
a60abd0
to
d6f3cd8
Compare
39bc46e
to
a8cd60e
Compare
2a0c2d4
to
47d004a
Compare
47d004a
to
0d5b1e1
Compare
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. |
Even for resolving conflicts? |
Yes, please save your efforts and the CI resources. |
@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 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. |
It's OK, it's only a matter of being inexperienced, and we (ZSF) have to balance mentorship with delivering a product. |
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 howstd.mem.page_size
fundamentally works. Before, we had hard coded every page size for how thebuiltin.target
value was "set up". Now the default behavior is to check ifbuiltin.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 tobuiltin.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.