Fixes #31229 fixes up BSD type mismatch errors and updates libc dependency part deux #31263
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
dhuseby
changed the title
| if stackp == MAP_FAILED { | ||
| panic!("failed to allocate an alternative stack"); | ||
| } | ||
| libc::stack_t { ss_sp: stackp as *mut i8, ss_flags: 0, ss_size: SIGSTKSZ } |
There was a problem hiding this comment.
I'd personally prefer to not duplicate these calls to mmap (they both return have the exact same call). I saw discussion about a stack_t on the other thread, but why's that done here?
There was a problem hiding this comment.
The reason this is duplicated is because there is only one mmap and it always returns a *mut libc::c_void. On FreeBSD and DragonflyBSD, the libc::stack_t.ss_sp member is a *mut i8. Also, because we have compile flags that say treat all warnings as errors, the FreeBSD and DragonflyBSD builds were failing.
What is needed is a conditional compilation block that casts the result of mmap to a *mut i8 when building for FreeBSD and DragonflyBSD. BUT, we don't have conditional compilation for general expressions yet. We only have it for functions. So I'm forced to wrap the mmap call and libc::stack_t initialization in a function that can be conditionally compiled based on OS.
There was a problem hiding this comment.
I don't understand why we need this at all, though, doesn't this work on all platforms?
let stackp = mmap(...);
if stackp == MAP_FAILED { ... }
return stackp as *mut u8There was a problem hiding this comment.
No it doesn't. There are compile flags that turn warnings into errors and this generates a warning.
There was a problem hiding this comment.
What flags are those? I thought we removed the lint for trivial casts... Either way a local #[allow] is better than all-out code duplication here I think
There was a problem hiding this comment.
The errors on FreeBSD look like this:
../src/libstd/sys/unix/stack_overflow.rs:151:23: 151:32 error: mismatched types:
expected `*mut i8`,
found `*mut libc::c_void`
(expected i8,
found enum `libc::c_void`) [E0308]
../src/libstd/sys/unix/stack_overflow.rs:151 stack.ss_sp = alt_stack;
^~~~~~~~~
There was a problem hiding this comment.
oh wait, i see what you're saying. maybe we get an error whenever we go from *mut libc::c_void to *mut i8 but we may not get an error going the other way. so if we have a get_stack() function that always return the stack pointer as *mut i8 then we eliminate the error above on platforms that have an i8 stack pointer. let me try that in the morning.
There was a problem hiding this comment.
Yeah if everything returns *mut foo then returning bar as *mut foo shouldn't warn
There was a problem hiding this comment.
@alexcrichton it didn't work. i changed the code to get_stack() -> *mut i8 for all platforms and now on non-freebsd, i'm getting:
../src/libstd/sys/unix/stack_overflow.rs:152:44: 152:55 error: mismatched types:
expected `*mut libc::c_void`,
found `*mut i8`
(expected enum `libc::c_void`,
found i8) [E0308]
../src/libstd/sys/unix/stack_overflow.rs:152 let stack = libc::stack_t { ss_sp: get_stack(), ss_flags: 0, ss_size: SIGSTKSZ };
i don't think your solution works. we should go with this patch instead.
There was a problem hiding this comment.
Right it's not working because this is using stack_t (and I don't know why). If that's not used and only pointers are used it shouldn't require duplication of the calls to mmap (which needs to be prioritized)
|
☔ The latest upstream changes (presumably #31285) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I think the merge conflict is from libc, which at this point should be fully upgraded, so that part can probably be backed out of this change |
|
@alexcrichton I backed out the libc update revision. If you accept my explanation for duplicating the call to mmap, then this PR should be GTG. |
|
@alexcrichton i rebased and rearranged the code so that there is only a single call to mmap. this is simpler than previous solutions, but still relies on conditional compilation. |
Something went haywire with github last night and the old PR #31230 got closed somehow. This new PR is to replace the old one. This incorporates all of the feedback from the other PR. @alexcrichton I incorporated the suggestion from @semarie and the result is cleaner and clearer. I think this is ready to go.
Something went haywire with github last night and the old PR #31230 got closed somehow. This new PR is to replace the old one. This incorporates all of the feedback from the other PR.
@alexcrichton I incorporated the suggestion from @semarie and the result is cleaner and clearer. I think this is ready to go.