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.

Sign up for GitHub

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

Jump to bottom

Enable and make stage0 landing pads optional #28717

Merged
merged 2 commits into from
Oct 5, 2015
Merged

Enable and make stage0 landing pads optional #28717

merged 2 commits into from
Oct 5, 2015

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Sep 28, 2015

Part of #28710

Landing pads during stage0 are now enabled by defaullt. Since this has its downsides and upsides either way, I made it possible to change the option through configure.

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

cc @arielb1, thoughts?

alexcrichton
alexcrichton reviewed Sep 28, 2015
View reviewed changes
configure
@@ -578,6 +578,7 @@ opt ccache 0 "invoke gcc/clang via ccache to reuse object files between builds"
opt local-rust 0 "use an installed rustc rather than downloading a snapshot"
opt llvm-static-stdcpp 0 "statically link to libstdc++ for LLVM"
opt rpath 0 "build rpaths into rustc itself"
opt landing-pads 1 "enable landing pads during bootstrap"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could be called stage0-landing-pads to ensure it's not mistaken for disabling landing pads entirely?

@solson solson mentioned this pull request Sep 29, 2015
Closed
@nagisa nagisa force-pushed the optional-no-landing-pads branch 2 times, most recently from 91d2bc0 to faef226 Compare September 29, 2015 14:52
@nagisa
Copy link
Member Author

nagisa commented Sep 29, 2015

Whatever way this PR goes, IMHO it still would be great to fix compiletest to work with or without landing-pads.

@nagisa nagisa force-pushed the optional-no-landing-pads branch from faef226 to 3f03032 Compare September 29, 2015 14:54
alexcrichton
alexcrichton reviewed Sep 29, 2015
View reviewed changes
mk/main.mk
@@ -170,7 +170,10 @@ RUST_LIB_FLAGS_ST3 += -C prefer-dynamic

# Landing pads require a lot of codegen. We can get through bootstrapping faster
# by not emitting them.
RUSTFLAGS_STAGE0 += -Z no-landing-pads

ifdef CFG_DISABLE_STAGE1_LANDING_PADS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be called stage0 to mirror the option it's used for below? I guess it kinda depends on what a "stage1 bootstrap" means, which also depends on who you talk to! May be worth just keeping these numbers in sync though to alleviate confusion of the off-by-one nature?

@alexcrichton
Copy link
Member

IMHO it still would be great to fix compiletest to work with or without landing-pads

Certainly!

@nagisa nagisa force-pushed the optional-no-landing-pads branch from 3f03032 to cefadf0 Compare September 29, 2015 17:18
@alexcrichton
Copy link
Member

@bors: r+ cefadf0

Thanks!

@arielb1
Copy link
Contributor

arielb1 commented Sep 29, 2015

@alexcrichton

This is a decent interim solution, but I would prefer a real one.

@bors
Copy link
Collaborator

bors commented Sep 30, 2015

⌛ Testing commit cefadf0 with merge e1f7470...

@bors
Copy link
Collaborator

bors commented Sep 30, 2015

💔 Test failed - auto-win-msvc-64-opt

@arielb1
Copy link
Contributor

arielb1 commented Oct 1, 2015

@bors retry

@arielb1
Copy link
Contributor

arielb1 commented Oct 1, 2015

@bors r-

@nagisa
Copy link
Member Author

nagisa commented Oct 1, 2015

I got no way to investigate this failure, sadly.

I could make pads disabled by default, but that makes this PR pretty useless IMO.

@alexcrichton
Copy link
Member

I think that unwinding for MSVC 64-bit landed sometime after the last snapshot causing these problems (e.g. MSVC 64-bit in stage0 wants the old method of unwinding). If you tweak these cfgs to have 64-bit MSVC in stage0 routed to seh.rs and only routed to seh64_gnu.rs in stage1+ I think it'll solve that error

@nagisa
Copy link
Member Author

nagisa commented Oct 2, 2015

@alexcrichton can we get new snapshots instead?

@alexcrichton
Copy link
Member

@bors: r+ 8cf5cf0

If we can avoid new snapshots for now it'd probably be good to keep doing so, but if we really need to we could surely make new snapshots!

@bors
Copy link
Collaborator

bors commented Oct 3, 2015

⌛ Testing commit 8cf5cf0 with merge 3f78603...

@bors
Copy link
Collaborator

bors commented Oct 3, 2015

💔 Test failed - auto-win-gnu-64-opt

@nagisa
Copy link
Member Author

nagisa commented Oct 3, 2015

Well, at least MSVC didn’t fail, but it seems wiser to wait for snaps rather than trying to fix all windows’ landing pad implementations for stage0.

@alexcrichton
Copy link
Member

Oh right, I think in stage0 x86_64-pc-windows-gnu needs to be routed to gcc.rs instead of seh64_gnu.rs as well. I think that'll do the trick though?

@nagisa nagisa force-pushed the optional-no-landing-pads branch from 8cf5cf0 to 67c2d2a Compare October 4, 2015 20:45
@nagisa
Copy link
Member Author

nagisa commented Oct 4, 2015

OK, lets retry this.

@alexcrichton
Copy link
Member

Looks like the travis failure may be legit

@nagisa nagisa force-pushed the optional-no-landing-pads branch from 67c2d2a to 594b2a3 Compare October 5, 2015 06:01
@bluss
Copy link
Member

bluss commented Oct 5, 2015

@bors try

@bors
Copy link
Collaborator

bors commented Oct 5, 2015

⌛ Trying commit 594b2a3 with merge c682cff...

bors added a commit that referenced this pull request Oct 5, 2015
@bors
Auto merge of #28717 - nagisa:optional-no-landing-pads, r=<try>
c682cff 
Part of #28710

Landing pads during stage0 are now enabled by defaullt. Since this has its downsides and upsides either way, I made it possible to change the option through configure.
@bluss
Copy link
Member

bluss commented Oct 5, 2015

@bors try-

@bluss
Copy link
Member

bluss commented Oct 5, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 5, 2015

📌 Commit 594b2a3 has been approved by bluss

@alexcrichton
Copy link
Member

@bors: retry force

@bors
Copy link
Collaborator

bors commented Oct 5, 2015

⌛ Testing commit 594b2a3 with merge d319a17...

@bors
Copy link
Collaborator

bors commented Oct 5, 2015

💔 Test failed - auto-win-gnu-64-opt

@alexcrichton
Copy link
Member

Aha, looks like stage0 64-bit gnu windows needs seh64_gnu.rs (support had landed at that time), it must have just been after the snapshot that support was added for MSVC. Routing it to that unwinding strategy gets the bootstrap farther into stage0 for me.

@nagisa nagisa force-pushed the optional-no-landing-pads branch from 594b2a3 to a7f2a78 Compare October 5, 2015 20:50
@alexcrichton
Copy link
Member

@bors: r+ a7f2a78

@bors
Copy link
Collaborator

bors commented Oct 5, 2015

⌛ Testing commit a7f2a78 with merge 6843ea4...

bors added a commit that referenced this pull request Oct 5, 2015
@bors
Auto merge of #28717 - nagisa:optional-no-landing-pads, r=alexcrichton
6843ea4 
Part of #28710

Landing pads during stage0 are now enabled by defaullt. Since this has its downsides and upsides either way, I made it possible to change the option through configure.
@bors
Copy link
Collaborator

bors commented Oct 5, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit a7f2a78 into rust-lang:master Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nagisa @rust-highfive @alexcrichton @arielb1 @bors @bluss