Rename no_split_stack to no_stack_check, and add a -C option by kmcallister · Pull Request #17037 · rust-lang/rust

kmcallister · 2014-09-06T01:51:43Z

alexchandel · 2014-09-06T02:44:03Z

Test failures:

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

kmcallister · 2014-09-06T02:53:36Z

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 · 2014-09-06T03:25:24Z

My bad, I suppose it's passing then.

huonw · 2014-09-07T05:18:36Z

Needs a rebase.

kmcallister · 2014-09-07T19:02:33Z

Rebased.

brson · 2014-09-08T18:08:30Z

@alexcrichton Are you ok with this?

alexcrichton · 2014-09-09T14:35:57Z

Looks good to me!

alexchandel · 2014-09-11T14:51:04Z

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 · 2014-09-17T01:18:16Z

@kmcallister Any ideas?

alexcrichton · 2014-09-29T14:39:32Z

ping @kmcallister, any updates on this?

alexchandel · 2014-10-09T05:13:04Z

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 · 2014-10-09T17:38:30Z

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 · 2014-10-09T19:39:06Z

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

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.

Fixes rust-lang#16980.

kmcallister · 2014-10-09T22:18:14Z

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.

kmcallister · 2014-10-10T00:12:30Z

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

@brson

r? @brson Fixes #16980.

…=Veykril internal: improve `TokenSet` implementation and add reserved keywords The current `TokenSet` type represents "A bit-set of `SyntaxKind`s" as a newtype `u128`. Internally, the flag for each `SyntaxKind` variant in the bit-set is set as the n-th LSB (least significant bit) via a bit-wise left shift operation, where n is the discriminant. Edit: This is problematic because there's currently ~121 token `SyntaxKind`s, so adding new token kinds for missing reserved keywords increases the number of token `SyntaxKind`s above 128, thus making this ["mask"](https://github.com/rust-lang/rust-analyzer/blob/7a8374c162c64c17e865b98aad282d16b16e96d6/crates/parser/src/token_set.rs#L31-L33) operation overflow. ~~This is problematic because there's currently 266 SyntaxKinds, so this ["mask"](https://github.com/rust-lang/rust-analyzer/blob/7a8374c162c64c17e865b98aad282d16b16e96d6/crates/parser/src/token_set.rs#L31-L33) operation silently overflows in release mode.~~ ~~This leads to a single flag/bit in the bit-set being shared by multiple `SyntaxKind`s~~. This PR: - Changes the wrapped type for `TokenSet` from `u128` to `[u64; 3]` ~~`[u*; N]` (currently `[u16; 17]`) where `u*` can be any desirable unsigned integer type and `N` is the minimum array length needed to represent all token `SyntaxKind`s without any collisions~~. - Edit: Add assertion that `TokenSet`s only include token `SyntaxKind`s - Edit: Add ~7 missing [reserved keywords](https://doc.rust-lang.org/stable/reference/keywords.html#reserved-keywords) - ~~Moves the definition of the `TokenSet` type to grammar codegen in xtask, so that `N` is adjusted automatically (depending on the chosen `u*` "base" type) when new `SyntaxKind`s are added~~. - ~~Updates the `token_set_works_for_tokens` unit test to include the `__LAST` `SyntaxKind` as a way of catching overflows in tests.~~ ~~Currently `u16` is arbitrarily chosen as the `u*` "base" type mostly because it strikes a good balance (IMO) between unused bits and readability of the generated `TokenSet` code (especially the [`union` method](https://github.com/rust-lang/rust-analyzer/blob/7a8374c162c64c17e865b98aad282d16b16e96d6/crates/parser/src/token_set.rs#L26-L28)), but I'm open to other suggestions or a better methodology for choosing `u*` type.~~ ~~I considered using a third-party crate for the bit-set, but a direct implementation seems simple enough without adding any new dependencies. I'm not strongly opposed to using a third-party crate though, if that's preferred.~~ ~~Finally, I haven't had the chance to review issues, to figure out if there are any parser issues caused by collisions due the current implementation that may be fixed by this PR - I just stumbled upon the issue while adding "new" keywords to solve rust-lang#16858~~ Edit: fixes rust-lang#16858

Keegan McAllister added 3 commits October 9, 2014 14:24

Rename the no_split_stack attribute to no_stack_check

db3bd23

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.

Add -C no-stack-check

d7fff9f

Fixes rust-lang#16980.

Add tests for no-stack-check attr and codegen option

4417968

Disable no-stack-check test on Windows

bc3831b

bors added a commit that referenced this pull request Oct 10, 2014

auto merge of #17037 : kmcallister/rust/no-stack-check, r=thestinger

45797a0

r? @brson Fixes #16980.

bors closed this Oct 10, 2014

Uh oh!

Conversation

kmcallister commented Sep 6, 2014

Uh oh!

alexchandel commented Sep 6, 2014

Uh oh!

kmcallister commented Sep 6, 2014

Uh oh!

alexchandel commented Sep 6, 2014

Uh oh!

huonw commented Sep 7, 2014

Uh oh!

kmcallister commented Sep 7, 2014

Uh oh!

brson commented Sep 8, 2014

Uh oh!

alexcrichton commented Sep 9, 2014

Uh oh!

alexchandel commented Sep 11, 2014

Uh oh!

alexchandel commented Sep 17, 2014

Uh oh!

alexcrichton commented Sep 29, 2014

Uh oh!

alexchandel commented Oct 9, 2014

Uh oh!

kmcallister commented Oct 9, 2014

Uh oh!

alexchandel commented Oct 9, 2014

Uh oh!

kmcallister commented Oct 9, 2014

Uh oh!

kmcallister commented Oct 10, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants