Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove obsolete no_stack_check attribute and test #34915

Closed
infinity0 opened this Issue Jul 19, 2016 · 19 comments

Comments

Projects
None yet
@infinity0
Copy link
Contributor

infinity0 commented Jul 19, 2016

Here is the start of the failure

failures:

---- [debuginfo-gdb] debuginfo-gdb/function-prologue-stepping-no-stack-check.rs stdout ----


executing armv7-unknown-linux-gnueabihf/stage2/bin/rustc /home/infinity0/rust/src/test/debuginfo/function-prologue-stepping-no-stack-check.rs -L armv7-unknown-linux-gnueabihf/test/debuginfo-gdb/ --target=armv7-unknown-linux-gnueabihf -L armv7-unknown-linux-gnueabihf/test/debuginfo-gdb/function-prologue-stepping-no-stack-check.stage2-armv7-unknown-linux-gnueabihf.debuginfo-gdb.libaux -C prefer-dynamic -o armv7-unknown-linux-gnueabihf/test/debuginfo-gdb/function-prologue-stepping-no-stack-check.stage2-armv7-unknown-linux-gnueabihf -C link-args=-Wl,-z,relro --cfg rtopt -C rpath -L armv7-unknown-linux-gnueabihf/rt -g
------stdout------------------------------

------stderr------------------------------
/home/infinity0/rust/src/test/debuginfo/function-prologue-stepping-no-stack-check.rs:253:1: 253:18 warning: unused attribute, #[warn(unused_attributes)] on by default
/home/infinity0/rust/src/test/debuginfo/function-prologue-stepping-no-stack-check.rs:253 #[no_stack_check]
                                                                                         ^~~~~~~~~~~~~~~~~
/home/infinity0/rust/src/test/debuginfo/function-prologue-stepping-no-stack-check.rs:270:1: 270:18 warning: unused attribute, #[warn(unused_attributes)] on by default
/home/infinity0/rust/src/test/debuginfo/function-prologue-stepping-no-stack-check.rs:270 #[no_stack_check]
                                                                                         ^~~~~~~~~~~~~~~~~
[..] many of these, basically one for every instance of no_stack_check in the file

After this basically everything is broken which you can see in more detail here. However, I see that no_stack_check isn't used in the code anywhere any more - there is only one grep result for stack[-_]check that excludes this test itself, and one of them says this:

src/libsyntax/feature_gate.rs-    // Not used any more, but we can't feature gate it
src/libsyntax/feature_gate.rs:    ("no_stack_check", Normal, Ungated),

So I wonder if this test should just be gotten rid of?

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jul 19, 2016

The test probably should go.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jul 19, 2016

I think its also a time to deprecate the attribute by now. cc @rust-lang/compiler @rust-lang/lang

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jul 19, 2016

If the attribute doesn't have any effect anymore, the test case should be removed (while making sure that's it's still possible to break on a function, which might mean that we have to write another test case if that isn't covered anywhere else).

@infinity0

This comment has been minimized.

Copy link
Contributor Author

infinity0 commented Jul 21, 2016

fwiw, this test does pass on x86, x86-64 and even arm64

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jul 21, 2016

It seems odd that the test is failing in this context, but not in the other targets that @infinity0 mentioned.

Nonetheless, the compiler team concluded we should:

  1. kill this test,
  2. kill this attribute,
  3. kill any compiler flag that exposing a similar thingie.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 21, 2016

I'll happily mentor someone to do this.

@brson brson added the E-easy label Jul 22, 2016

@brson brson changed the title debuginfo/function-prologue-stepping-no-stack-check fails on armv7 hf Delete obsolete no_stack_check attribute and test Jul 22, 2016

@brson brson changed the title Delete obsolete no_stack_check attribute and test Remove obsolete no_stack_check attribute and test Jul 22, 2016

@abhijeetbhagat

This comment has been minimized.

Copy link
Contributor

abhijeetbhagat commented Jul 23, 2016

I can work on this.

@abhijeetbhagat

This comment has been minimized.

Copy link
Contributor

abhijeetbhagat commented Jul 25, 2016

So this is as simple as just deleting /src/test/debuginfo/function-prologue-stepping-regular.rs and removing this:
// Not used any more, but we can't feature gate it
("no_stack_check", Normal, Ungated),
from /src/libsyntax/feature_gate.rs ?

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jul 25, 2016

You’ll also need to remove this flag. Getting the things you mentioned along with this flag deleted sounds correct.

@abhijeetbhagat

This comment has been minimized.

Copy link
Contributor

abhijeetbhagat commented Jul 25, 2016

I created three different pull requests because i did not know how to create a single pull request for multiple files. How can i do this in the future?

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jul 25, 2016

I created three different pull requests because i did not know how to create a single pull request for multiple files. How can i do this in the future?

Did you perhaps make the PRs using github’s edit file feature? I do not think that feature was ever meant for anything more than quickly changing a typo in a single file.

The proper workflow (which allows for multi-file patches as well) is to:

  • Fetch the project to your device;
  • Change the files (as many of them as you want);
  • Commit the changes to your local repository using any of the git tools available for your platform. Github has its own GUI application for Windows and Mac as well;
  • Push your commit(s) to your github fork;
  • Create a PR for your commit(s).

There a number of tutorials on how to get started with this workflow on the internet. In no particular order: 1, 2, 2.5, 3, 4, 5.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 25, 2016

bors added a commit that referenced this issue Jul 26, 2016

Auto merge of #35028 - abhijeetbhagat:patch-6, r=alexcrichton
Remove no_stack_check tests (#34915)

Part of fixes for #34915

steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 26, 2016

@pmatos

This comment has been minimized.

Copy link
Contributor

pmatos commented Sep 23, 2016

What's still missing here?

@lfairy

This comment has been minimized.

Copy link
Contributor

lfairy commented Oct 22, 2016

@pmatos AFAIK, the only thing left to do is have -C no-stack-check print a deprecation warning when used. There was a pull request for this at #35156, but it was closed due to inactivity.

eddyb added a commit to eddyb/rust that referenced this issue Nov 9, 2016

Rollup merge of rust-lang#37636 - karpinski:issue-34915, r=nikomatsakis
Marking the 'no-stack-check' codegen option as deprecated (Issue rust-lang#34915)

Attempts to finish resolving issue rust-lang#34915. Based on pull request rust-lang#35156, which was closed due to inactivity.
@mgattozzi

This comment has been minimized.

Copy link
Member

mgattozzi commented Jan 13, 2017

This issue should be closed now right? It mentions in the release notes of #38966 that this has been fixed and it seems like #34915 was the one that closed the issue.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jan 13, 2017

@mgattozzi Yes, looks like it. Thanks for the reminder!

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jan 13, 2017

No. We still need to remove the flag. Please reopen.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jan 13, 2017

Oh, I thought the issue was just about the test, sorry.

@danobi

This comment has been minimized.

Copy link
Contributor

danobi commented Feb 25, 2017

This issue was fixed in PR #37636. I believe this should be closed.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 2, 2017

per @danobi's comment, closing

jdhorwitz added a commit to jdhorwitz/rust that referenced this issue Mar 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.