Conversation
824f901 to
c62a394
Compare
c62a394 to
d78b3c6
Compare
matu3ba
left a comment
There was a problem hiding this comment.
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
andrewrk
left a comment
There was a problem hiding this comment.
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
993943d to
f1429dc
Compare
|
PR is ready, just rebasing. |
3cf2f2e to
7e088e4
Compare
|
Looks like |
41c81e9 to
da0086a
Compare
|
|
||
| /// Emits log messages for leaks and then returns whether there were any leaks. | ||
| pub fn detectLeaks(self: *Self) bool { | ||
| self.ensureInitialized() catch unreachable; |
There was a problem hiding this comment.
Uh, catch unreachable on allocation failure is not ok.
There was a problem hiding this comment.
Would a panic be better or would a silent failure and return that there are no leaks be better?
There was a problem hiding this comment.
If the GPA is not initialized, surely we can immediately return that there were no leaks, since no memory was allocated?
Either way, yes, catch unreachable is not acceptable. unreachable means "control flow can never reach this location; if it does, that is programmer error". Outside of specific cases like some uses of FBA, error.OutOfMemory is never an unreachable case.
There was a problem hiding this comment.
Essentially, solving this needs higher level design work for the GPA to meet the new constraint of page size being runtime known.
There was a problem hiding this comment.
What would that entail?
There was a problem hiding this comment.
Ok, I have it return false on failure now. Looks good?
There was a problem hiding this comment.
@ifreund Pinging you to see if the change is better since it's been some time since I've heard back.
There was a problem hiding this comment.
Answering the question of how this API should look would require me to do the higher level design work I mentioned, which I don't have time for.
There was a problem hiding this comment.
Well then the return false when the initialization fails in the leak detection should be good enough for this PR, could always update this once you or someone else has the time.
ifreund
left a comment
There was a problem hiding this comment.
Thanks for your efforts working on this branch!
Due to the tricky nature of these changes, a Zig core team member will need to take this branch over and directly test the changes in order to get this improvement landed. I can provide feedback on these changes, but this issue is blocking on a core team member taking it on and I personally have other priorities at the moment.
176c23a to
0b235aa
Compare
4997c4b to
53eff28
Compare
a60abd0 to
d6f3cd8
Compare
d6f3cd8 to
a2ad81c
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_sizefundamentally works. Before, we had hard coded every page size for how thebuiltin.targetvalue was "set up". Now the default behavior is to check ifbuiltin.target.page_sizeis 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.