Remove the fixed_stack_segment attribute
#10155
Comments
|
I don't get it. Why can't we just remove the whole notion of fixed_stack_segment altogether? Why would we generate it at codegen time (what does it even mean?) In any case, how can we possibly guarantee that there is (e.g.) 2MB of stack, when we can't allocate more? It's true that calling native code can fall off the end of the stack but...well...that's why it's considered unsafe. |
|
(Also, I should note that guard pages would help us to die a more controlled death here) |
|
We still need the idea of a large stack segment for when calling FFI. We should at least attempt to make calls to C "safe" by guarnateeing that there's at least 2MB of stack for it to run on (or some large number). The rust function would then request 2MB of space, and if there wasn't that much left it would trigger an overflow (and currently abort). We'll always suffer from native code falling off the stack, but it's probably true that most realistic native code won't fall off 2 MB of stack. |
|
I still don't understand what the idea of a large stack segment brings to the table here. It offers no guarantees and will create many false "stack overflow" reports. Worse, there is no way for users to recover -- with stack growth we at least "fell back" to wasting a lot of memory. Without stack growth, you just get an error, even if the C code would have ran fine (which is likely). If it's safety we're after, let's use a guard page instead. Then you only get an error IF your C code actually overflows. One of the appealing parts of dropping stack growth was dropping all the complexity around FFI calls. If we wind up keeping that complexity, I may want to revisit my opinion about dropping stack growth in the first place. =) |
|
I thought that we were going to keep the Before I made |
|
The fact that we need 4MB stacks to work around the check suggests to me that it is limiting -- in other words, it's forcing us to allocate a very large stack in order to ensure that we have a very big buffer at the end so we can make native calls, much larger than ought to be necessary. As far as complexity, I am mostly worried about end-user complexity, not implementation complexity. I agree it's no great challenge to flag those functions which call extern "C" fns, but we can't just uniformly say that they require 2MB of stack. We're going to need the ability to flag fns that require smaller amounts so that they can be used in contexts where users want to explicitly allocate smaller stack frames etc. Re: morestack vs guard pages, I personally am ok with just having morestack for now, and accepting that calling a native fn can segfault. I'm curious to hear others' opinions though. |
|
accepted for P-high. We know what to do after tuesday mtg, just need to do it. |
|
Guard pages are essential (since there's no guarantee the C function will only try to use 4MB or any fixed stack amount) and of course already provided by all OSes since otherwise C code can corrupt the heap by running off the stack, and C compilers obviously insert checks if the stack is larger than 4KB by touching all pages. However, the check is still necessary if one wants to make the task fail instead of aborting the process on stack overflow, since you can't unwind C code if it causes a stack overflow because it's not guaranteed to be exception safe (and thus you need to unwind before the call). |
|
You can't unwind pure Rust when it hits the stack overflow either, LLVM doesn't consider |
|
It seems to me that we should consider adding an LLVM intrinsic for "stack size of current function" or something like that. In that case, we could insert guards ourselves as normal LLVM code. This would also allow us to optimize the placement of stack checks (i.e., move them up the stack chain as far as possible). One complication though is inlining, which can result in redundant checks, though we might be able to teach LLVM to optimize those away as part of GVN/PRE/CSE/whatever-version-of-those-optimizations-LLVM-performs. One tricky part to optimizing stack checks is that the failure would occur in somewhat unpredictable places to the user, but I think in practice that's ok. (That is, if I have X that (unconditionally) calls Y that (unconditionally) calls Z, and Z would run out of stack, X might be the one to fail.) But whenever you run out of stack there is always a recursive function and in reality one always just looks up the stack at a near infinite series of frames that fail randomly somewhere in the cycle anyhow, so changing the point in the cycle where this occurs is not a big deal. |
|
It doesn't seem like it's worth keeping around the segmented stack checks if we're not going to treat FFI calls in a special way. Almost all Rust code makes FFI calls, so an infinitely recursive function has a good chance of not being memory safe. It will get close to the boundary and then all it takes is one |
…_lint, r=Manishearth Add new lint for byte char slices This patch adds a new lint that checks for potentially harder to read byte char slices: `&[b'a', b'b']` and suggests to replace them with the easier to read `b"ab"` form. Fixes rust-lang#10147 --- changelog: new lint: [`byte_char_slices`] [rust-lang#10155](rust-lang/rust-clippy#10155) <!-- changelog_checked -->
In today's meeting we have decided to jettison segmented stacks. This means that there's no need for this attribute because the purpose of it was to lift the stack requirements higher in the callstack to prevent thrashing.
Removing this isn't just as simple as removing it entirely, as we still need to make sure that any FFI call requires a minimum of 2MB of stack remaining. This means that codegen-wise we will have to continue adding the
fixed-stack-segmentattribute, but the compiler will do this in an automated fashion now instead of requiring users to do it manually.Additionally, this also means that the
externfn!macro should get removed (it was just used to generate the fixed stack segment wrapper).Nominating for 1.0, this would be awesome to remove.
Note that existing issues had the following problems:
fixed-stack-segmentattributefixed-stack-segmentattributeThe text was updated successfully, but these errors were encountered: