gimli-rs / unwind-rs

Apache License 2.0
26 stars 10 forks source link

[DONTMERGE] Aarch64 unwinding support #7

Open roblabla opened 6 years ago

roblabla commented 6 years ago

Hey,

This is not ready to be merged yet, but I figured I'd submit the PR early to get some feedback.

So I added a partially working unwinder for AArch64 (there are still a few bugs lurking, RUST_BACKTRACE=1 causes a segfault for some reason, trace segfaults). It runs the example demo properly though.

In order to restore callee-saved SIMD Vector registers, I had to bump the size of Registers from 32 to 96. This causes a bit of a problem: Registers now have an overhead of 96 * 8 = 768 bytes. I feel like that might be a bit huge.

In truth, I don't need that many bytes: many of the registers in that space are reserved. I only have 64 actual registers that need to be stored, the "holes" are unused. I'm thinking that maybe I should merge LandingRegisters and Registers, and have DwarfRegister map to a field inside LandingRegisters ? Or maybe this is all premature optimization. I'm not sure :).

Now I just need to figure out why RUST_BACKTRACE=1 causes a segfault. For some reason, in the _Unwind_Backtrace closure, the trace callback becomes invalid (it contains a pointer on the stack instead of a function pointer). I suppose I'm overflowing the stack somewhere :eyes:

main-- commented 6 years ago

First of all, I really like what you did with the asm/glue code. Obviously I can't comment much on the AArch64 specifics as I'm not familiar with them. What stuck out to me though is that you changed Registers to zero-initialize all registers. Is there a specific reason for this?

I'm thinking that maybe I should merge LandingRegisters and Registers, and have DwarfRegister map to a field inside LandingRegisters ? Or maybe this is all premature optimization. I'm not sure :).

I think finding the right way to abstract the register state is a hard problem, especially when it comes to something like cross-platform compatibility. Perhaps a trait based solution may be the right choice (DwarfUnwinder<R: RegisterState>)? I'm not sure. If you want, feel free to come up with something that works well. But for now I think this is fine the way it is - we can always worry about performance later on.

roblabla commented 6 years ago

Fun: https://github.com/rust-lang/rust/issues/52836. This is what causes the segfault I believe. Guess I'll move to global_asm.

roblabla commented 6 years ago

What stuck out to me though is that you changed Registers to zero-initialize all registers. Is there a specific reason for this?

If you mean https://github.com/roblabla/unwind-rs/blob/652553568bc8d2ee89bd05d17bbacdc04e35c367/unwind/src/registers.rs#L9, that's totally a mistake. Should have put None in there - or even better, called out to Default::default(). Fixing it.


I think I'll first PR something that works properly, and then have a follow-up PR refactoring Registers in something with a bit less overhead. I'm personally interested in having something low-overhead because the context I'll be using this in might have a fairly small stack (I believe 8KB), so storing excess data might cause issues.

roblabla commented 6 years ago

Fixed a few more bugs. We are now triggering an assert while unwinding the stack with RUST_BACKTRACE=1.

TRACE:unwind: fde aaaaaaad40d8 - 2c, address aaaaaaad40bb
thread 'main' panicked at 'assertion failed: fde.contains(address)', unwind/src/lib.rs:131:13

It doesn't cause issues with RUST_BACKTRACE=0, because the unwinding stops before it hits the problematic frame.

EDIT : The problematic frame seems to be the _start function. Hmm.

roblabla commented 6 years ago

So, if I'm reading this right, the FDE returned by eh_frame_hdr.table().lookup() is wrong. Which is super odd. I wonder if there's any tool to print/test the contents of eh_frame_hdr? Dwarfdump doesn't seem to have anything for it.

main-- commented 6 years ago

If you mean https://github.com/roblabla/unwind-rs/blob/652553568bc8d2ee89bd05d17bbacdc04e35c367/unwind/src/registers.rs#L9, that's totally a mistake. Should have put None in there - or even better, called out to Default::default(). Fixing it.

In that case, why are you manually implementing Default at all? (since you removed the derive)

I think I'll first PR something that works properly, and then have a follow-up PR refactoring Registers in something with a bit less overhead.

Sure, sounds good.

The problematic frame seems to be the _start function.

This is a tricky problem. From what I remember glibc's __libc_start_main is properly set up with CFI to stop the unwinder on x86_64. Not sure what's going wrong for you here.

I wonder if there's any tool to print/test the contents of eh_frame_hdr? Dwarfdump doesn't seem to have anything for it.

Since it's not really DWARF it makes sense that dwarfdump doesn't support it. I'm not aware of any tools, I just stared at hexdumps.

roblabla commented 6 years ago

I'm implementing default manually because derive(Default) doesn't work for arrays bigger than 32. That's also why I removed derive(PartialEq) and derive(Eq). I didn't bother reimplementing those because they were unused as far as I could see.

main-- commented 6 years ago

Oh, of course, I forgot about that.

roblabla commented 6 years ago

So yeah, I checked both the .eh_frame_hdr (through tons of logs) and .eh_frame (through dwarfdump), and the _start function has no FDE entry there. When using system unwinder, the backtrace looks like below, with the last frame set as . The function seems to expect the unwinder to get to it: it sets up a proper start frame by setting x30 and x29 to 0.

I'm going to attempt compiling gcc's libunwind with tracing information, and compile the demo against it. Maybe that will reveal what's wrong.

stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:475
   5: std::panicking::begin_panic
             at /checkout/src/libstd/panicking.rs:409
   6: demo::bar
             at unwind/examples/demo.rs:13
   7: demo::foo
             at unwind/examples/demo.rs:18
   8: demo::main
             at unwind/examples/demo.rs:24
   9: std::rt::lang_start::{{closure}}
             at /checkout/src/libstd/rt.rs:74
  10: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  11: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:106
  12: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:392
             at libstd/rt.rs:58
  13: std::rt::lang_start
             at /checkout/src/libstd/rt.rs:74
  14: main
  15: __libc_start_main
  16: <unknown>
roblabla commented 6 years ago

So, I managed to make a stopgap solution. I made _Unwind_Backtrace propagate the error when it fails to find unwind information (instead of just panicking). The rust side ignores errors and prints the backtrace anyways. So that works for my use-case... But it's pretty ugly...

I've played around with various libraries. GCC's libunwind sadly has no debug. However, I can tell its _Unwind_Backtrace returns END_OF_STACK. Nongnu's libunwind returns ENOINFO, so it has the same problem we do. I haven't tried llvm's Libunwind, it's next on my list.

roblabla commented 5 years ago

Hey. So I'm currently working on a 32-bit x86 port of the unwinder. This code is kinda old, and I'm not quite sure what's necessary to bring it upstream. I'm thinking of submitting a subset of this PR that just refactors the existing code to be backend agnostic, on top of which this PR (and a new one) will build to add aarch64 and x86 support.

main-- commented 5 years ago

Considering that I've been tied up in other projects for quite some time now now and will be for the near future at least I'm open to merging pretty much any reasonable patch. In other words, feel free to submit whatever you think makes sense and I will gladly review and merge all of it.