Skip to content
This repository was archived by the owner on Feb 5, 2019. It is now read-only.

[Sparc] Account for bias in stack readjustment #94

Merged
merged 1 commit into from
Jan 29, 2018
Merged

[Sparc] Account for bias in stack readjustment #94

merged 1 commit into from
Jan 29, 2018

Conversation

glaubitz
Copy link

While working on bootstrapping rustc for sparc64, Michael Karcher discovered that LLVM may generate code where the stack alignment on sparc64 is incorrect which results in rustc segfaulting on sparc64 [1].

Michael created a crude hack which fixed the issue and therefore proved his theory. James Clarke later came up with a proper patch which he has submitted upstream [2].

I have tested both Michael's and James' patch and both fix the segfault of rustc on sparc64. Since Michael's patch does deal with alignment larger than 4k and may also not fix corrupted stack pointers, we chose to use
James' patch.

The patch has already been merged into the llvm-toolchain-4.0 package [3].

Thanks,
Adrian

[1] http://lists.llvm.org/pipermail/llvm-dev/2017-October/118620.html
[2] https://reviews.llvm.org/D39425
[3] https://anonscm.debian.org/viewvc/pkg-llvm?view=revision&revision=2849

@glaubitz
Copy link
Author

CC @jrtc27 @karcherm

@alexcrichton
Copy link
Member

Thanks for the PR! Is this in upstream LLVM yet?

@glaubitz
Copy link
Author

It has been submitted upstream, but not merged yet.

See: https://reviews.llvm.org/D39425

@alexcrichton
Copy link
Member

Ok! Let's hold off on merging until this is upstream?

@glaubitz
Copy link
Author

That's up to you.

We just won't be able to build a working compiler on sparc64/sparcv9 before this fix has been merged.

@alexcrichton
Copy link
Member

Ok yeah it's just standard procedure for us to wait until patches like this are upstream before merging.

@glaubitz
Copy link
Author

glaubitz commented Jan 23, 2018

@alexcrichton It has been merged upstream now, see:

https://reviews.llvm.org/D39425

I have updated the branch to include the changes @jrtc27 made to the testsuite. So the change matches what upstream merged now.

Can you pull the change?

@jrtc27
Copy link

jrtc27 commented Jan 23, 2018

Actually it still hasn't been merged, just approved; I've pinged the review again.

@glaubitz
Copy link
Author

Since this patch affects sparc64 only and since it unbreaks the Rust compiler on Linux/sparc64, can we maybe get an exception for getting it merged?

It's the only things that keeps us from having a working Rust compiler on Linux/sparc64 at the moment.

@glaubitz
Copy link
Author

glaubitz commented Jan 29, 2018

@alexcrichton It has been merged now upstream. Could we get this merged here now as well?

See: https://reviews.llvm.org/rL323643

@alexcrichton alexcrichton merged commit a9c93bc into rust-lang:rust-llvm-release-4-0-1 Jan 29, 2018
@alexcrichton
Copy link
Member

Sure thing, thanks for getting this upstreamed!

@glaubitz
Copy link
Author

Thanks a lot! I just saw that there is a 6.0 LLVM branch now. Should I open a PR for this branch as well so the patch lands there, too?

@alexcrichton
Copy link
Member

Ah no need! I went ahead an put the commit on the branch

@glaubitz
Copy link
Author

Wow, you were so fast that it was already there when I just pulled to check whether's it's necessary :D.

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

Successfully merging this pull request may close these issues.

3 participants