Uh oh!
There was an error while loading. Please reload this page.
Flush executables to disk after linkage - #49432
Conversation
rust-highfive
commented
Mar 27, 2018
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
michaelwoerister
commented
Mar 28, 2018
Thanks for the PR, @nabijaczleweli! Could you please
|
nabijaczleweli
commented
Mar 28, 2018
Done |
michaelwoerister
commented
Mar 29, 2018
Looks great now, thank you! Please squash the commits into one, then this should be good to go. |
A problem caused by not doing so in Chrome has been reported here: https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/amp/ File::sync_all() calls FlushFileBuffers() down the line, causing potentially unflushed buffers on high I/O-load systems to flush and prevent nasty non-reproducible bugs. The force-flush is only done on Windows and if the linker exited successfully Closes#48545
nabijaczleweli
commented
Mar 29, 2018
Done |
ollie27
commented
Mar 29, 2018
This would also need to be done here: rust/src/librustc_trans/back/link.rs Lines 837 to 838 in ae544ee Although I'm not convinced that it wouldn't be better to rely on the linker to perform this mitigation. |
Done. As for "the linker" doing so – probably a good idea, but unreliable (henlo systems running 5-year-old |
michaelwoerister
commented
Mar 30, 2018
Thanks @nabijaczleweli and @ollie27! @bors r+ |
bors
commented
Mar 30, 2018
📌 Commit 3787106 has been approved by |
bors
commented
Mar 30, 2018
⌛ Testing commit 3787106 with merge 64f93f31b12940a6a4e083191fbaf5c1a479106a... |
bors
commented
Mar 30, 2018
💔 Test failed - status-appveyor |
nabijaczleweli
commented
Mar 31, 2018
Error seems to've originated outside of this PR's scope 🤔 |
ollie27
commented
Mar 31, 2018
|
This should be enough and shouldn't require append(true) since we're not explicitly writing anything so we're not flushing it so we've no risk of overwriting it
nabijaczleweli
commented
Mar 31, 2018
I was aware of that logically, but it's slipped my fingers. Anyway, fixed by using |
michaelwoerister
commented
Apr 3, 2018
@bors r+ |
bors
commented
Apr 3, 2018
📌 Commit e1d3c47 has been approved by |
…rister Flush executables to disk after linkage A problem caused by not doing so in Chrome has been reported [here](https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/amp/). `File::sync_all()` calls `FlushFileBuffers()` down the line, causing potentially unflushed buffers on high I/O-load systems to flush and preventing nasty non-reproducible bugs. Closesrust-lang#48545
…rister Flush executables to disk after linkage A problem caused by not doing so in Chrome has been reported [here](https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/amp/). `File::sync_all()` calls `FlushFileBuffers()` down the line, causing potentially unflushed buffers on high I/O-load systems to flush and preventing nasty non-reproducible bugs. Closesrust-lang#48545
Rollup of 9 pull requests Successful merges: - #48658 (Add a generic CAS loop to std::sync::Atomic*) - #49253 (Take the original extra-filename passed to a crate into account when resolving it as a dependency) - #49345 (RFC 2008: Finishing Touches) - #49432 (Flush executables to disk after linkage) - #49496 (Add more vec![... ; n] optimizations) - #49563 (add a dist builder to build rust-std components for the THUMB targets) - #49654 (Host compiler documentation: Include private items) - #49667 (Add more features to rust_2018_preview) - #49674 (ci: Remove x86_64-gnu-incremental builder) Failed merges:
A problem caused by not doing so in Chrome has been reported here.
File::sync_all()callsFlushFileBuffers()down the line, causing potentially unflushed buffers on high I/O-load systems to flush and preventing nasty non-reproducible bugs.Closes#48545