rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
537 stars 246 forks source link

Fix get_sp implementation for s390x #389

Closed uweigand closed 3 years ago

uweigand commented 3 years ago

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 commented 3 years ago

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

uweigand commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/rust-lang/backtrace-rs/pull/391

uweigand commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/rust-lang/backtrace-rs/pull/392. It was indeed a bit more difficult than I initially expected, see the comment over there for details.

alexcrichton commented 3 years ago

👍

uweigand commented 3 years ago

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 commented 3 years ago

Indeed! I went ahead and published 0.3.55 too :)

uweigand commented 3 years ago

Works for me, thanks again!