-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
stack overflow handler specific openbsd change. #87528
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@@ -143,11 +143,17 @@ mod imp { | |||
} | |||
|
|||
unsafe fn get_stackp() -> *mut libc::c_void { | |||
#[cfg(target_os = "openbsd")] | |||
let flags = MAP_PRIVATE | MAP_ANON | libc::MAP_STACK; | |||
// OpenBSD requires this flag for stack mapping |
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.
"this flag" is referring to MAP_STACK
? If so the comment is on the wrong line.
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.
indeed :)
The PR title and description could use some improvement. After all every commit implicitly is a change. And why is it needed? Is OpenBSD broken without it or is it only a correctness fix that currently doesn't make a difference but may do so in the future? If it's the latter shouldn't it also be applied on linux since the manpage there says that
|
0, | ||
); | ||
// OpenBSD requires this flag for stack mapping | ||
// otherwise the said mapping will fail is a no-op in most systems |
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.
s/is/as
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.
s/in/on
reads nicer I think.
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.
sure thing
#[cfg(target_os = "openbsd")] | ||
let flags = MAP_PRIVATE | MAP_ANON | libc::MAP_STACK; | ||
#[cfg(not(target_os = "openbsd"))] | ||
let flags = MAP_PRIVATE | MAP_ANON; |
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.
Linux supports MAP_STACK
too. It is a no-op on all architectures. FreeBSD supports it too. For example macOS doesn't support it though.
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.
yes but not necessary on these oses plus solaris/illumos does not have it and I believe haiku neither.
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.
yes but not necessary
Not currently necessary. A future architecture may need it. That is why it exists as no-op at all.
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.
ok can add for linux, netbsd but I m less sure about FreeBSD the flag is working but does not seem to work well with null start addresses, seems little usage difference.
On this platform, when doing stack allocation, MAP_STACK is needed otherwise the mapping fails.
Seems reasonable, and consistent with the OpenBSD documentation. @bors r+ |
📌 Commit 853ffc7 has been approved by |
stack overflow handler specific openbsd change.
Appears to not work with all Unix libcs? #89529
@bors r- |
it might needs a libc update ... it is present since the 0.2.102 release (or updating the current PR to not enabling on netbsd). |
having lost the original repo since, I opted for libc update, anyway it would have been needed at some point. |
to solve rust-lang#87528 build.
…d, r=dtolnay library std, libc dependency update to solve rust-lang#87528 build.
…d, r=dtolnay library std, libc dependency update to solve rust-lang#87528 build.
The |
📌 Commit 853ffc7 has been approved by |
stack overflow handler specific openbsd change.
stack overflow handler specific openbsd change.
…laumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#75644 (Add 'core::array::from_fn' and 'core::array::try_from_fn') - rust-lang#87528 (stack overflow handler specific openbsd change.) - rust-lang#88436 (std: Stabilize command_access) - rust-lang#89614 (Update to Unicode 14.0) - rust-lang#89664 (Add documentation to boxed conversions) - rust-lang#89700 (Fix invalid HTML generation for higher bounds) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
No description provided.