-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Align android sigaddset
impl with the reference impl from Bionic
#100782
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors r+ rollup |
…rk-Simulacrum Align android `sigaddset` impl with the reference impl from Bionic In rust-lang#100737 I noticed we were treating the sigset_t as an array of bytes, while referencing code from android (https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h) which treats it as an array of unsigned long. That said, the behavior difference is so subtle here that it's not hard to see why nobody noticed. This fixes the implementation to be equivalent to the one in bionic.
…rk-Simulacrum Align android `sigaddset` impl with the reference impl from Bionic In rust-lang#100737 I noticed we were treating the sigset_t as an array of bytes, while referencing code from android (https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h) which treats it as an array of unsigned long. That said, the behavior difference is so subtle here that it's not hard to see why nobody noticed. This fixes the implementation to be equivalent to the one in bionic.
@bors r- failed in a rollup |
@bors try (I think this includes the stuff that failed?) |
⌛ Trying commit 4ecf876 with merge 5b1f98a3aa6cd874cb1f46edf817e39e6ffb89c6... |
This failed on the |
Huh, I thought try ran the same tests that run on rollup (I guess I just assumed this. in my defense, this is more or less what try was used for on firefox). Is there an easy way to do that? |
I guess I can make it fail here with rollup=never, and at least this way it won't cause someone else trouble. Ideally I'd have a local android dev env set up, but mine broke a few months ago and I haven't gotten around to it. @bors r=Mark-Simulacrum rollup=never |
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward |
Ouff, do we still have bug that when we |
Finished benchmarking commit (5b1f98a3aa6cd874cb1f46edf817e39e6ffb89c6): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
OK, please don't try and then r+ in the future (until rust-lang/homu#47 is fixed). Otherwise I think @bors r=Mark-Simulacrum rollup=never is reasonable. We can make try builds run android with some work -- there's some documentation here: https://rustc-dev-guide.rust-lang.org/tests/ci.html#using-ci-to-test |
💡 This pull request was already approved, no need to approve it again.
|
Noted, thanks, I didn't realize that bug existed. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1cff564): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Footnotes |
In #100737 I noticed we were treating the sigset_t as an array of bytes, while referencing code from android (https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h) which treats it as an array of unsigned long.
That said, the behavior difference is so subtle here that it's not hard to see why nobody noticed. This fixes the implementation to be equivalent to the one in bionic.