nbdd0121 / unwinding

Stack unwinding library in Rust
Apache License 2.0
102 stars 18 forks source link

Initial libstd support #17

Closed xobs closed 1 year ago

xobs commented 1 year ago

This PR contains two patches required to allow unwinding to be used with libstd.

As background, I'm porting libstd to the Xous operating system. This is an embedded target.

In the past I've applied patches to libunwind in order to get it to work, however those patches are very difficult to get upstream. A recent change in llvm made C compiling difficult (https://reviews.llvm.org/D156642), and additional patches will be needed on top of Rust's hacked-up version of LLVM that adds support for SGX.

With these patches, along with an unwinding backend for the Rust unwind crate, I'm able to run tests and catch panics in Xous.

This patchset does two things:

  1. Add support for making core optional, which is necessary for building as part of the tree
  2. Make text_base optional, which is required due to the fact that Xous doesn't have this information and no platforms currently use it anyway

While it's true that (2) is an API-level change, this API is not documented in either docs.rs or the examples/ directory, so I'm not sure how widely used it is.

nbdd0121 commented 1 year ago

I believe the base address can be zero. Probably just using Option<usize> is fine?

xobs commented 1 year ago

Good point -- I've adjusted the API accordingly.

nbdd0121 commented 1 year ago

CI is failing due to rust-lang/rust#108955 denying lang_items feature and rust-lang/rust#112849 changing the panic display output. Fix in progress...

nbdd0121 commented 1 year ago

On the implementation side; do you think it makes sense to just use 0 as text_base when it's unknown instead of None? If textrel is not actually used, then setting it to 0 wouldn't actually cause any problem? I am a little bit reluctant to make this Option because it'll be a semver-breaking change.

I think you mentioned in Zulip the issue is that LLVM doesn't expose the start of text section by default? Could you give more context about this, and how do you define the custom FDE finder? We might be able to find a solution to use one of the existing FDE finder or get the text base using some other method.

MasterAwesome commented 1 year ago

How does the text_base currently get set for a platform that requires this to be non-zero?

Also it makes sense for this to be a usize instead of an Option<usize> due the reason Some(0) and None have the same meaning. Instead if you do use Option<NonZeroUSize> to keep the ABI same due to the guaranteed NPO that might be better. What do you think?

nbdd0121 commented 1 year ago

If you try to get textrel base when it's not set, you get a panic (which will get turned into abort), so it's not exactly the same as getting a zero.

However it was pointed out that textrel base isn't actually being used and LLVM libunwind does not support it, so it won't have any observable difference. GCC's unwinder does support textrel and will report 0 if not set, but from some digging it seems that it was only used by IA64 and some other non-mainstream architectures and they are no longer generating it.

MasterAwesome commented 1 year ago

If you try to get textrel base when it's not set, you get a panic (which will get turned into abort), so it's not exactly the same as getting a zero.

Ah I see; thanks for the explanation. How does it impact whether it's set or not; is it purely during what IP is returned? In which case if it isn't supported anymore is the caller expected to be doing the textrel base offsetting to get the non-reloced address? Are there reasons on why the design choice was made by llvm to not support it?

However it was pointed out that textrel base isn't actually being used and LLVM libunwind does not support it, so it won't have any observable difference. GCC's unwinder does support textrel and will report 0 if not set, but from some digging it seems that it was only used by IA64 and some other non-mainstream architectures and they are no longer generating it.

That makes sense, in that case I can see why Option does make sense here to denote the fact that it can be set or not. What do you think about the Option<NonZeroUsize> idea since gcc's set but 0 means the same thing in this case.

nbdd0121 commented 1 year ago

No, Option<NonZeroUsize> doesn't make sense because the textrel base can be set and be zero.

xobs commented 1 year ago

I've rebased on top of trunk.

I think that Option<usize> is the best option going forward, but I don't know what to do about existing targets. I see two dependencies on crates.io, neither of which uses the custom unwinder.

What if all of the unwinders were changed to Option<usize> and this made a 0.3.0 release? I believe that's allowed under semver, since it's 0.x.y.

nbdd0121 commented 1 year ago

I'll take this as part of 0.2.0 release. There're some functions that are not supposed to be safe exposed in unwinding::abi and I'd like to fix these, which also is semver breaking change anyway.