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

Expose each frame's canonical frame address (CFA) #339

Closed
fitzgen opened this issue May 28, 2020 · 16 comments
Closed

Expose each frame's canonical frame address (CFA) #339

fitzgen opened this issue May 28, 2020 · 16 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented May 28, 2020

In libunwind, this is _Unwind_GetCFA.

Pretty sure other unwinders will also expose this API.

Worst case scenario, we can go to the parent frame and ask for its stack pointer, which will be the base of the child frame 😆

@alexcrichton
Copy link
Member

Done in #341!

@uweigand
Copy link
Contributor

@fitzgen @alexcrichton
This is causing problems on s390x, because we use a biased CFA: the CFA of a frame equals the SP at function entry plus 160.

Now the code that was added as #341 introduces a method called "get_sp" which is implemented via _Unwind_GetCFA on the next inner frame. This is only correct if the CFA of a function equals the SP at function entry, which is not true on s390x.

Now, I'm not completely sure what the right way to fix this is. If the intent of the function is actually to return the SP, then we'll need a different implementation on s390x. (I have a patch working that uses _Unwind_GetGR(ctx, 15) instead of _Unwind_GetCFA.)

However, if the intent is really to return the CFA then maybe the function should be renamed? Also, in that case the users (at least the one in wasmtime) needs to fixed then.

What do you suggest?

@alexcrichton
Copy link
Member

I believe the intention turned out to be "expose a function to get the stack pointer register" and leave it up to applications to figure out how to infer the CFA from that. Seems fine though to have a s390x-specific part of the code which changes the implementation here!

@bjorn3
Copy link
Member

bjorn3 commented Nov 17, 2020

I think CFA is actually the right thing to expose. Unlike the stack pointer it is guaranteed to be stable for a single frame. The stack pointer may be changed by the compiler at any point. Where does Wasmtime need the stack pointer?

@uweigand
Copy link
Contributor

I think CFA is actually the right thing to expose. Unlike the stack pointer it is guaranteed to be stable for a single frame. The stack pointer may be changed by the compiler at any point. Where does Wasmtime need the stack pointer?

Wasmtime uses the stack pointer during garbage collection, where it looks up stack maps. It is indeed the case that the stack pointer can change, but wasmtime creates stack maps that take the current SP into account. If this is changed to be CFA-relative instead, then the stack map creation needs to be updated accordingly.

@uweigand
Copy link
Contributor

I believe the intention turned out to be "expose a function to get the stack pointer register" and leave it up to applications to figure out how to infer the CFA from that. Seems fine though to have a s390x-specific part of the code which changes the implementation here!

I see the discussion is still ongoing, but there's the implementation of a get_sp appropriate for s390x, if this is the way to go:
#389

@fitzgen
Copy link
Member Author

fitzgen commented Nov 17, 2020

  • Yes, stack maps are SP-relative, so that's what we want. I agree that in an ideal world they would be CFA-relative, but we don't actually have a good way of getting the proper CFA with libunwind because (see next bullet point)

  • The get_sp implementation here calls _Unwind_GetCFA becuase libunwind, if you look at the source, confusingly implements this function by unconditionally getting the SP (which is often, but not always on all platforms, the previous frame's SP; it's a huge mess)

@uweigand
Copy link
Contributor

uweigand commented Nov 17, 2020

  • The get_sp implementation here calls _Unwind_GetCFA becuase libunwind, if you look at the source, confusingly implements this function by unconditionally getting the SP (which is often, but not always on all platforms, the previous frame's SP; it's a huge mess)

The implementation in libunwind also seems buggy, that's true. It actually doesn't get used however (at least on typical Linux platforms), because nobody links against libunwind. Instead, we get _Unwind_GetCFA together with all the other _Unwind routines from the system libgcc (or sometimes glibc). At least there, _Unwind_GetCFA actually does return the correct CFA.

As to which frame is involved, _Unwind_CFA returns the CFA of the current frame (i.e. the one where you pass the context in), while all the other routines, in particular _Unwind_GetIP and _Unwind_GetGR return register values from the previous frame.

[ Therefore, it does make sense on most platforms to implement _Unwind_CFA (determine CFA of the current frame) in terms of unwinding the SP of the previous frame, because on most platforms the CFA of the current frame does indeed equal the value of SP on function entry, which is the SP of the previous frame. But again this is not correct on s390x. ]

@fitzgen
Copy link
Member Author

fitzgen commented Nov 17, 2020

I think the backtrace crate ends up linking against the libunwind in Rust's std, not the system libunwind. Maybe @alexcrichton can confirm.

@uweigand
Copy link
Contributor

Well, when I was debugging it today, I was stepping into libgcc, not any version of libunwind.

@uweigand
Copy link
Contributor

Just checked again, and on my system the "wasmtime" binary I get from "cargo build" resolves all the _Unwind symbols dynamically against libgcc_s.so:

[uweigand@t35lp56 wasmtime]$ nm -D target/debug/wasmtime | grep _Unwind
                 U _Unwind_Backtrace
                 U _Unwind_FindEnclosingFunction
                 U _Unwind_GetDataRelBase
                 U _Unwind_GetGR
                 U _Unwind_GetIP
                 U _Unwind_GetIPInfo
                 U _Unwind_GetLanguageSpecificData
                 U _Unwind_GetRegionStart
                 U _Unwind_GetTextRelBase
                 U _Unwind_RaiseException
                 U _Unwind_Resume
                 U _Unwind_SetGR
                 U _Unwind_SetIP
[uweigand@t35lp56 wasmtime]$ ldd target/debug/wasmtime
        linux-vdso64.so.1 (0x000003fffdffe000)
        libdl.so.2 => /lib64/libdl.so.2 (0x000003fffbd00000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x000003fffbc80000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x000003fffbc00000)
        libc.so.6 => /lib64/libc.so.6 (0x000003fffba00000)
        /lib/ld64.so.1 (0x000003fffdf80000)
        libm.so.6 => /lib64/libm.so.6 (0x000003fffb900000)

Not sure if this was the intent, but that's what happening right now (at least on my system) ...

@fitzgen
Copy link
Member Author

fitzgen commented Nov 17, 2020

Very interesting! When I was debugging a few months ago I was definitely getting the SP-as-the-"CFA" behavior! Can't remember what lib it was coming from, but what I was seeing was consistent with the libunwind sources.

I vaguely remember libgcc embedding its own vendored version of libunwind...

Anyways, cleaning all this stuff up would definitely be nice!

@uweigand
Copy link
Contributor

For the _Unwind routines, libgcc is definitely the "canonical" source. Sometimes that version is also "vendored" into glibc.

Libunwind primarily defines its own API, but then also implements the _Unwind routines, but I think this is only really intended to be used for compatibility on systems that don't have libgcc (or glibc).

In any case, as I said above, on most (non-s390x) platforms the previous-SP-as-CFA implementation is actually correct.

@fitzgen
Copy link
Member Author

fitzgen commented Nov 17, 2020

the previous-SP-as-CFA implementation is actually correct.

Yes, this would be correct, but libunwind returns the current frame's SP, not the previous frame's SP!

Anyways, if you can clean up the implementation and make it make sense, please go for it!

@uweigand
Copy link
Contributor

Yes, this would be correct, but libunwind returns the current frame's SP, not the previous frame's SP!

Hmm, I don't see that -- the libunwind implementation of _Unwind_GetCFA is
unw_get_reg (&context->cursor, UNW_REG_SP, &val);
but unw_get_reg actually returns the value of a register in the previous frame; in that respect it works just like _Unwind_GetGR (as you can also see by the fact that _Unwind_GetGR is implemented simply as unw_get_reg in libunwind!).

@alexcrichton
Copy link
Member

For better or worse there's like 100 copies of libunwind but most of the time each platform uses only one of them. Rust's musl target, for example, should use libunwind for statically linked binaries. macOS additionally uses libunwind from the system. As you've seen though the libgcc version is used for most other Linux targets.

I don't really have a preference of what API to export here or what platforms it's supported on. It seems inevitable that this crate has the union of all platform capabilities and then documents specifically that each function only works on some platforms and not others. As implementations are figured out and/or become available we can fill things in though.

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

No branches or pull requests

4 participants