Skip to content

std.mem: Don't use vectorized path for freestanding targets in indexOfSentinel() #23182

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented Mar 10, 2025

This removes the invalid page size assumptions made in std.mem.indexOfSentinel() for freestanding targets. This is what originally prompted the commits that I'm reverting, which were added in #22488 and #23043. See the discussion: #22488 (review)

See also #23184.

@alexrp alexrp marked this pull request as draft March 10, 2025 11:04
@alexrp alexrp marked this pull request as ready for review March 10, 2025 11:41
@alexrp alexrp requested a review from andrewrk March 10, 2025 11:41
@alexrp alexrp added this to the 0.14.1 milestone Mar 10, 2025
@alexrp alexrp changed the title std: Remove invalid page size assumptions std.mem: Don't use vectorized path for freestanding targets in indexOfSentinel()` Mar 10, 2025
@alexrp alexrp changed the title std.mem: Don't use vectorized path for freestanding targets in indexOfSentinel()` std.mem: Don't use vectorized path for freestanding targets in indexOfSentinel() Mar 10, 2025
@andrewrk
Copy link
Member

What makes the page size assumptions invalid?

Also this is not a bug fix, this is breaking.

@andrewrk andrewrk removed this from the 0.14.1 milestone Mar 11, 2025
@alexrp
Copy link
Member Author

alexrp commented Mar 12, 2025

What makes the page size assumptions invalid?

The key points from the previous discussion are:

  • For freestanding, it's invalid to assume that paging is even enabled. But, even if it is, it may very well not be at the exact time that some standard library code such as std.mem.indexOfSentinel() is called.
  • The standard library's notion of page size is not the same as the CPU architecture's. The former happens to be a more narrow subset of the latter because OSs are generally sane, but there is no iron law stating that this must be the case; you can create really weird illusions with page protection and code resumption tricks, for example.
    • The idea is to ultimately let the OS be the source of truth for what page sizes are possible, or in the absence of an OS, have the user tell us. This works out because, per the first point, we have to assume that paging is a meaningless concept on freestanding unless the user knows better.

Also this is not a bug fix, this is breaking.

Only 4f43918 needs to be picked into 0.14.1 to fix std.mem.indexOfSentinel() for freestanding. The other commits are just reverting the changes to std.heap.page_size_min/page_size_min_default because the case that prompted them is no longer relevant; these can just go in 0.15.0 since they're technically breaking. We're tagging 0.14.1 soon, so not including this anyway.

@alexrp alexrp added this to the 0.14.1 milestone Mar 26, 2025
@alexrp alexrp removed this from the 0.14.1 milestone Apr 26, 2025
@alexrp alexrp force-pushed the page-size-assumptions branch from 4f43918 to fc88592 Compare May 4, 2025 08:48
@alexrp alexrp force-pushed the page-size-assumptions branch from fc88592 to ba2c859 Compare May 28, 2025 09:50
@alexrp alexrp force-pushed the page-size-assumptions branch from ba2c859 to dfbdef9 Compare June 14, 2025 02:06
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