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 the return_address intrinsic. #34491

Merged
merged 1 commit into from Jun 27, 2016
Merged

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 26, 2016

This intrinsic to get the return pointer was introduced in #16248 / #16081 by @pcwalton for Servo.
However, as explained in #34227, it's impossible to ensure it's used correctly, and it broke with -Zorbit.

Servo's usage is being replaced in servo/servo#11872, and I expect nobody else to have abused it.
But I've also started a crater run, just in case this is a [breaking-change] for anyone else.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@nagisa
Copy link
Member

nagisa commented Jun 27, 2016

r=me provided crater is happy. In case it isn’t we may want to introduce a cycle of warning instead?

@eddyb
Copy link
Member Author

eddyb commented Jun 27, 2016

Crater report shows 0 regressions.

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jun 27, 2016

📌 Commit b30134d has been approved by nagisa

@bors
Copy link
Contributor

bors commented Jun 27, 2016

⌛ Testing commit b30134d with merge af2fe63...

bors added a commit that referenced this pull request Jun 27, 2016
Remove the return_address intrinsic.

This intrinsic to get the return pointer was introduced in #16248 / #16081 by @pcwalton for Servo.
However, as explained in #34227, it's impossible to ensure it's used correctly, and it broke with `-Zorbit`.

Servo's usage is being replaced in servo/servo#11872, and I expect nobody else to have abused it.
But I've also started a crater run, just in case this is a `[breaking-change]` for anyone else.
@bors bors merged commit b30134d into rust-lang:master Jun 27, 2016
@eddyb eddyb deleted the return-in-peace branch June 27, 2016 05:25
@aturon
Copy link
Member

aturon commented Jun 28, 2016

I'm surprised this went through without more (apparent) approval from stakeholders. I realize that there's work underway to move Servo off this intrinsic, but until that work lands, doesn't this effectively break Servo? We are usually much more careful about breaking production uses of Rust, and something like this would usually merit at least a ping to @rust-lang/compiler and @rust-lang/lang. But maybe everyone is in the loop through other forums?

cc @Manishearth @metajack @jdm -- just want to make sure you're aware of and OK with taking this step prior to actually landing the requisite changes in Servo.

@jdm
Copy link
Contributor

jdm commented Jun 28, 2016

My preference would be to revert this until we've finished removing its usage from Servo.

Edit: Thanks for checking with us, by the way!

@nagisa
Copy link
Member

nagisa commented Jun 28, 2016

I was under impression that servo/servo#11872 has landed but apparently it didn’t and only was approved.

To my knowledge, though, servo doesn’t use the most recent nightly anyway and the Servo’s PR seems like will land in near future?

@Manishearth
Copy link
Member

I was somewhat aware of this (bit too late though). I am sort of okay with this landing because Servo pins anyway and we can delay a rustup for a week or two (indeed, we usually don't rustup in that timeframe anyway). Hopefully the servo PR lands and works without issues.

Reverting is also okay by me. AIUI this isn't blocking anything.

@jdm
Copy link
Contributor

jdm commented Jun 28, 2016

I would rather not rush the review of the Servo change, and I would also like to avoid any potential of blocking anybody who needs to update Servo's Rust version while the review is in progress.

@nikomatsakis
Copy link
Contributor

Note PR to revert here #34580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants