Use control-flow information to improve handling of returns #19898
Conversation
|
r? @pnkfelix |
|
r+ with some of the set operation simplifications |
|
@alexcrichton do you know what's up with that failure? |
|
Hm no, can't say I've seen that one before! We do still have some known issues with linking and privacy, however. |
|
At the very least, I'm pretty sure it's not my fault. |
| @@ -49,4 +49,11 @@ impl CFG { | |||
| blk: &ast::Block) -> CFG { | |||
| construct::construct(tcx, blk) | |||
| } | |||
|
|
|||
| pub fn node_is_reachable(&self, id: ast::NodeId) -> bool { | |||
| for node in self.graph.depth_traverse(self.entry) { | |||
There was a problem hiding this comment.
Could be:
self.graph.depth_traverse.any(|node| node.id == id)This narrows the definition of nested returns such that only when the outer return has a chance of being executed (due to the inner return being conditional) do we mark the function as having nested returns. Fixes rust-lang#19684
|
Ok, probably was my fault. Given the error that caused it though, I'm amazed it managed to get that far into execution. |
|
@alexcrichton @cmr updated, should work now. |
|
For posterity, this diff also fixed the OSX-specific error that this was seeing. This is likely needed for correctness regardless and we're just getting lucky! That being said, I think the fix you have provided right now is the correct fix. diff --git a/src/librustrt/lib.rs b/src/librustrt/lib.rs
index 02ca7d3..e4935a5 100644
--- a/src/librustrt/lib.rs
+++ b/src/librustrt/lib.rs
@@ -27,6 +27,7 @@
extern crate alloc;
extern crate libc;
extern crate collections;
+extern crate unicode;
#[cfg(test)] extern crate "rustrt" as realrustrt;
#[cfg(test)] extern crate test;
@@ -81,6 +82,7 @@ pub fn init(argc: int, argv: *const *const u8) {
collections::fixme_14344_be_sure_to_link_to_collections();
alloc::fixme_14344_be_sure_to_link_to_collections();
libc::issue_14344_workaround();
+ unicode::issue_14344_workaround();
}
/// Enqueues a procedure to run when the runtime is cleaned up
diff --git a/src/libunicode/lib.rs b/src/libunicode/lib.rs
index 1f75daa..b83a2cc 100644
--- a/src/libunicode/lib.rs
+++ b/src/libunicode/lib.rs
@@ -82,3 +82,6 @@ mod std {
pub use core::clone;
pub use core::cmp;
}
+
+#[doc(hidden)]
+pub fn issue_14344_workaround() {} |
|
Also I don't personally feel confident enough to review this, so I'll have to defer to other trans wizards. |
|
I tested this out, and it sped up my |
|
@Aatch: looks like it failed on windows. It looks like a test timed out or maybe fell into an infinite loop? I can't tell if on my phone if this was a spurious error or not. |
rust-lang#16081 fixed an issue where a nested return statement would cause incorrect behaviour due to the inner return writing over the return stack slot that had already been written too. However, the check was very broad and picked many cases that wouldn't ever be affected by this issue. As a result, the number of allocas increased dramatically and therefore stack-size increased. LLVM is not able to remove all of the extraneous allocas. Any code that had multiple return values in a compound expression at the end of a function (including loops) would be hit by the issue. The check now uses a control-flow graph to only consider the case when the inner return is executed conditionally. By itself, this narrowed definition causes rust-lang#15763 to return, so the control-flow graph is also used to avoid passing the return slot as a destination when the result won't be used. This change allows the stack-size of the main rustc task to be reduced to 8MB from 32MB.
|
The error here happened twice in a row and I haven't seen it much on the other builders, so it may not be spurious. @Aatch if you can't come up with anything though we can always try it again! |
|
Hmm, it looks like the example in sync/task_pool.rs is the problem. It shouldn't be affected by the PR though, so I'm at a loss for an explanation. Especially since it works fine on every other platform. Even win64 I think |
#16081 fixed an issue where a nested return statement would cause incorrect behaviour due to the inner return writing over the return stack slot that had already been written too. However, the check was very broad and picked many cases that wouldn't ever be affected by this issue.
As a result, the number of allocas increased dramatically and therefore stack-size increased. LLVM is not able to remove all of the extraneous allocas. Any code that had multiple return values in a compound expression at the end of a function (including loops) would be hit by the issue.
The check now uses a control-flow graph to only consider the case when the inner return is executed conditionally. By itself, this narrowed definition causes #15763 to return, so the control-flow graph is also used to avoid passing the return slot as a destination when the result won't be used.
This change allows the stack-size of the main rustc task to be reduced to 8MB from 32MB.