Rename no_split_stack to no_stack_check, and add a -C option #17037

Closed
wants to merge 4 commits into
from

Projects

None yet

8 participants

@kmcallister
Contributor

r? @brson

Fixes #16980.

@alexchandel

Test failures:

...
test result: FAILED. 1242 passed; 7 failed; 23 ignored; 0 measured
@kmcallister
Contributor

Apparently the LLVM 3.4 tests always fail on Travis and it isn't an issue. But the 3.3 tests only ran for 36 seconds...

@alexchandel

My bad, I suppose it's passing then.

@huonw
Member
huonw commented Sep 7, 2014

Needs a rebase.

@kmcallister
Contributor

Rebased.

@brson
Contributor
brson commented Sep 8, 2014

@alexcrichton Are you ok with this?

@alexcrichton
Member

Looks good to me!

@alexchandel

Bors test failures:

maketest: no-stack-check
----- /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/src/test/run-make/no-stack-check/ --------------------
------ stdout ---------------------------------------------
DYLD_LIBRARY_PATH="/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/no-stack-check:/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/stage2/lib:" /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/stage2/bin/rustc --out-dir /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/no-stack-check -L /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/no-stack-check --emit asm attr.rs
! grep -q morestack /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/no-stack-check/attr.s

------ stderr ---------------------------------------------
make[1]: *** [all] Error 1
@alexchandel

@kmcallister Any ideas?

@alexcrichton
Member

ping @kmcallister, any updates on this?

@alexchandel

I rebased @kmcallister's branch locally and ran the tests (OS X). The problem is that, in src/test/run-make/no-stack-check, the no_stack_check function calls test::black_box, and test::black_box naturally still has a call to morestack. This call is detected by ! grep -q morestack $(TMPDIR)/attr.s which assumes the call came from the no_stack_check function and thus was erroneously generated, and the test fails.

Changing the call in attr.rs and flag.rs from test::black_box(x.as_slice()); to unsafe {asm!("" : : "r"(&x.as_slice()))} and adding #![feature(asm)], i.e. inlining test::black_box by hand, fixes this. @alexcrichton not sure if there is a better approach?

But this could be merged after a rebase and the aforementioned fix.

@kmcallister
Contributor

Wow, thanks for tracking that down! ๐Ÿ’š

Since the code doesn't need to pass the linker, I'll go with this solution:

#![crate_type="lib"]

extern {
    fn black_box(ptr: *const u8);
}

pub unsafe fn foo() {
    // Make sure we use the stack
    let x: [u8, ..50] = [0, ..50];
    black_box(x.as_ptr());
}
@alexchandel

No problem, I'm excited for this! :)

kmcallister added some commits Sep 6, 2014
@kmcallister kmcallister Rename the no_split_stack attribute to no_stack_check
The old name is misleading as we haven't had segmented stacks in quite some
time. But we still recognize it, with a deprecation warning.
db3bd23
@kmcallister kmcallister Add -C no-stack-check
Fixes #16980.
d7fff9f
@kmcallister kmcallister Add tests for no-stack-check attr and codegen option
4417968
@kmcallister
Contributor

I fixed the test issue and rebased. I also changed the flag description to "disable checks for stack exhaustion" to avoid confusion with features like -fstack-protect that check for overflows of buffers on the stack.

@alexcrichton

r+

Contributor
bors replied Oct 9, 2014

saw approval from alexcrichton
at kmcallister@4417968

Contributor
bors replied Oct 9, 2014

merging kmcallister/rust/no-stack-check = 4417968 into auto

Contributor
bors replied Oct 9, 2014

kmcallister/rust/no-stack-check = 4417968 merged ok, testing candidate = 27db701

cmr replied Oct 10, 2014

@bors: retry

Contributor
bors replied Oct 10, 2014

saw approval from alexcrichton
at kmcallister@4417968

Contributor
bors replied Oct 10, 2014

merging kmcallister/rust/no-stack-check = 4417968 into auto

Contributor
bors replied Oct 10, 2014

kmcallister/rust/no-stack-check = 4417968 merged ok, testing candidate = 787b33f

@kmcallister
Contributor

Some kind of network failure on Windows? I don't even know

@kmcallister kmcallister Disable no-stack-check test on Windows
bc3831b
@thestinger

r+

Is __chkstk affected by -C no-stack-check, and does it need to be tested?

Contributor

It's not impacted by it. I don't think there's a way to turn __chkstk off and I don't think there's a good reason to want it.

Contributor
bors replied Oct 10, 2014

saw approval from thestinger
at kmcallister@bc3831b

Contributor
bors replied Oct 10, 2014

merging kmcallister/rust/no-stack-check = bc3831b into auto

Contributor
bors replied Oct 10, 2014

kmcallister/rust/no-stack-check = bc3831b merged ok, testing candidate = 19c6afe

@bors: retry

Contributor
bors replied Oct 10, 2014

saw approval from thestinger
at kmcallister@bc3831b

Contributor
bors replied Oct 10, 2014

merging kmcallister/rust/no-stack-check = bc3831b into auto

Contributor
bors replied Oct 10, 2014

kmcallister/rust/no-stack-check = bc3831b merged ok, testing candidate = 45797a0

Contributor
bors replied Oct 10, 2014

fast-forwarding master to auto = 45797a0

@bors bors closed this Oct 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment