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

Fix personality_fn within the compiler_builtins #40254

Merged
merged 1 commit into from
Mar 9, 2017
Merged

Fix personality_fn within the compiler_builtins #40254

merged 1 commit into from
Mar 9, 2017

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Mar 4, 2017 •
edited
Loading

compiler_builtins may not have any unwinding within it to link correctly. This is notoriously
finicky, and this small piece of change removes yet another case where personality function
happens to get introduced.

Side note: I do remember solving the exact same thing before. I wonder why it has reappered...

@cuviper, could you please try building beta with this patch applied? It should apply cleanly. If it works, I’ll nominate to land this into beta.

Fixes(?) #40251

@rust-highfive
Copy link
Collaborator

r? @brson

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

@nagisa
Copy link
Member Author

nagisa commented Mar 4, 2017

(seems like a fallout of SNAP cleanup)

cuviper
cuviper reviewed Mar 4, 2017
View reviewed changes
src/libcompiler_builtins/lib.rs Outdated
fn iabs(self) -> i128 {
let s = self >> 127;
let s = self.wrapping_shl(127);
Copy link
Member

Choose a reason for hiding this comment

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

Should be wrapping_shr?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@nagisa
Fix personality_fn within the compiler_builtins
8f581cc 
compiler_builtins may not have any unwinding within it to link correctly. This is notoriously
finicky, and this small piece of change removes yet another case where personality function
happens to get introduced.

Side note: I do remember solving the exact same thing before. I wonder why it has reappered...
@nagisa nagisa force-pushed the compiler-builtin-no-panic branch from dc5fdbc to 8f581cc Compare March 4, 2017 09:29
@est31
Copy link
Member

est31 commented Mar 4, 2017

I don't see any conditional jump in the disassembly of that function. It would be very weird if it were the cause.

@est31
Copy link
Member

est31 commented Mar 4, 2017

... except if LLVM has a bug in their garbage collector.

@est31
Copy link
Member

est31 commented Mar 4, 2017

@nagisa I think the code was introduced by commit 29e01af . And IF it were introduced by the snap cleanup, it wouldn't be present on beta.

@cuviper
Copy link
Member

cuviper commented Mar 4, 2017

@cuviper, could you please try building beta with this patch applied? It should apply cleanly. If it works, I’ll nominate to land this into beta.

It doesn't apply cleanly, because of stage0 hackery, but it's straightforward enough. I'm having other troubles in my build root right now, and it's very late here, so I'll have to mess with it later...

@nagisa
Copy link
Member Author

nagisa commented Mar 4, 2017

@cuviper well, the point here is basically to move fn uabs body from the trait to impls here. I’ll make a PR against beta shortly.

@est31 I certainly remember us "fixing" similar issue where a trait function was called and introduced a personality_fn at one point.

nagisa added a commit to nagisa/rust that referenced this pull request Mar 4, 2017
@nagisa
Beta backport of rust-lang#40254
e8d90fb
@nagisa
Copy link
Member Author

nagisa commented Mar 4, 2017

#40256 is a patch against beta branch @cuviper

@cuviper
Copy link
Member

cuviper commented Mar 4, 2017

My i686 build works with #40256, thanks! I'm trying native builds now on all arches...

@cuviper
Copy link
Member

cuviper commented Mar 4, 2017

Everything looks good, thanks!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 6, 2017

📌 Commit 8f581cc has been approved by alexcrichton

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 6, 2017
bors added a commit that referenced this pull request Mar 7, 2017
@bors
Auto merge of #40256 - nagisa:compiler-builtin-no-panic-beta, r=alexc…
6e3a32a 
…richton

Beta backport of #40254
bors added a commit that referenced this pull request Mar 8, 2017
@bors
Auto merge of #40256 - nagisa:compiler-builtin-no-panic-beta, r=alexc…
92e7b9b 
…richton

Beta backport of #40254
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 8, 2017
Rollup merge of rust-lang#40254 - nagisa:compiler-builtin-no-panic, r…
1e53624 
…=alexcrichton

Fix personality_fn within the compiler_builtins

compiler_builtins may not have any unwinding within it to link correctly. This is notoriously
finicky, and this small piece of change removes yet another case where personality function
happens to get introduced.

Side note: I do remember solving the exact same thing before. I wonder why it has reappered...

@cuviper, could you please try building beta with this patch applied? It should apply cleanly. If it works, I’ll nominate to land this into beta.

Fixes(?) rust-lang#40251
@arielb1 arielb1 mentioned this pull request Mar 8, 2017
Merged
bors added a commit that referenced this pull request Mar 9, 2017
@bors
Auto merge of #40368 - arielb1:rollup, r=arielb1
3087a1f 
Rollup of 20 pull requests

- Successful merges: #40154, #40222, #40226, #40237, #40254, #40258, #40265, #40268, #40279, #40283, #40292, #40293, #40296, #40316, #40321, #40325, #40326, #40327, #40333, #40335
- Failed merges:
@bors bors merged commit 8f581cc into rust-lang:master Mar 9, 2017
@alexcrichton alexcrichton mentioned this pull request Mar 13, 2017
Merged
5 tasks
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 26, 2017
Closed
@est31 est31 mentioned this pull request Jul 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

Assignees

@brson brson

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nagisa @rust-highfive @est31 @cuviper @alexcrichton @bors @brson