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

Fix get_sp implementation for s390x #389

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Fix get_sp implementation for s390x #389

merged 1 commit into from
Nov 20, 2020

Conversation

uweigand
Copy link
Contributor

On s390x we use a biased CFA: the CFA of a function equal the SP
at function entry plus a constant of 160.

Therefore, the current implementation of get_sp, which uses
_Unwind_GetCFA, is not an appropriate way to retrieve the
current SP value on this platform.

Instead, use _Unwind_GetGR to explicitly retrieve the value
of the stack pointer register %r15.

On s390x we use a biased CFA: the CFA of a function equal the SP
at function entry plus a constant of 160.

Therefore, the current implementation of get_sp, which uses
_Unwind_GetCFA, is not an appropriate way to retrieve the
current SP value on this platform.

Instead, use _Unwind_GetGR to explicitly retrieve the value
of the stack pointer register %r15.
@alexcrichton
Copy link
Member

Looks good to me, thanks! Do you know if it would be possible to get s390x working on CI through qemu?

@uweigand
Copy link
Contributor Author

Looks good to me, thanks! Do you know if it would be possible to get s390x working on CI through qemu?

Probably. At least I was able to run the tests under the regular qemu provided by Ubuntu 20.04 using

CARGO_BUILD_TARGET=s390x-unknown-linux-gnu \
CARGO_TARGET_S390X_UNKNOWN_LINUX_GNU_LINKER=s390x-linux-gnu-gcc \
CARGO_TARGET_S390X_UNKNOWN_LINUX_GNU_RUNNER=qemu-s390x \
cargo test

It is a bit slow, but for a small package like backtrace-rs that's probably OK.

@uweigand
Copy link
Contributor Author

As an aside, I keep seeing this warning when building backtrace-rs (with or without my patch):

warning: hidden lifetime parameters in types are deprecated
   --> src/symbolize/gimli/elf.rs:164:68
    |
164 |     pub(super) fn search_object_map(&self, _addr: u64) -> Option<(&Context, u64)> {
    |                                                                    ^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
    |
note: the lint level is defined here
   --> src/lib.rs:51:9
    |
51  | #![warn(rust_2018_idioms)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(elided_lifetimes_in_paths)]` implied by `#[warn(rust_2018_idioms)]`

Is this expected?

@alexcrichton
Copy link
Member

Ah yeah if it's slightly slow that's fine! As for the warning it's fine to go ahead and fix, it's not intentional to have a warning!

@uweigand
Copy link
Contributor Author

Ah yeah if it's slightly slow that's fine! As for the warning it's fine to go ahead and fix, it's not intentional to have a warning!

OK, this is now #391

@uweigand
Copy link
Contributor Author

Can this PR be committed now, or is having the CI a pre-requisite? This does fix a crash in wasmtime GC on Z.

@alexcrichton
Copy link
Member

I'd ideally like to have CI since this has s390x-specific code, but if CI is difficult to add it can be handled later.

@uweigand
Copy link
Contributor Author

I'd ideally like to have CI since this has s390x-specific code, but if CI is difficult to add it can be handled later.

This is now #392. It was indeed a bit more difficult than I initially expected, see the comment over there for details.

@alexcrichton
Copy link
Member

👍

@alexcrichton alexcrichton merged commit 3405dc6 into rust-lang:master Nov 20, 2020
@uweigand uweigand deleted the s390x-get_sp branch November 20, 2020 17:22
@uweigand
Copy link
Contributor Author

Thanks, @alexcrichton !

In order to use this in wasmtime now, I guess I'll have to wait until a new version is released, right?

@alexcrichton
Copy link
Member

Indeed! I went ahead and published 0.3.55 too :)

@uweigand
Copy link
Contributor Author

Works for me, thanks again!

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.

2 participants