Page MenuHomePhabricator

RFC [libunwind] Convert the register restore functions to C functions
ClosedPublic

Authored by mstorsjo on Aug 16 2020, 1:35 PM.

Details

Summary

This fixes linking if built in MSVC mode. Even though the main unwinder interface isn't of much use in an MSVC environment, one might plausibly want to use libunwind for backtraces even there.

Only handled x86, x86_64 and arm64 so far. In particular, on arm32, it's not one function but a number of smaller functions.

Just posting this as a RFC. It simplifies the interface of the assembly somewhat, but might also require changes to the code for the cases where the calling convention differs between C and C++ functions (like on i386 windows).

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 16 2020, 1:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 16 2020, 1:35 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Aug 16 2020, 1:35 PM

The idea itself is reasonable. The summary description is very confusing would you mind reworking that? Could you check what happens for the jumpto member? Ideally it would just be an alias for the function to avoid a frame setup and unnecessary jump. I think that is the only thing that really stands out of needing work in the implementation.

The idea itself is reasonable. The summary description is very confusing would you mind reworking that?

Sure - does this sound more understandable?

[libunwind] Convert the register restore functions to C functions

Currently, the assembly functions for restoring register state have been direct implementations of the C++ method Registers_*::jumpto() (contrary to the functions for saving register state, which are implementations of the extern C function __unw_getcontext). This has included having the assembly function name match the C++ mangling of that method name (and having the function match the C++ member function calling convention). To simplify the interface between the C++ code and the assembly, make the assembly function have a plain C function interface.

This fixes building the library in with a MSVC C++ ABI, which uses a significantly different method name mangling scheme. (The library might not be of much use as C++ exception unwinder in such an environment, but the libunwind.h interface for stepwise unwinding still is usable, as is the _Unwind_Backtrace function.)

Could you check what happens for the jumpto member? Ideally it would just be an alias for the function to avoid a frame setup and unnecessary jump. I think that is the only thing that really stands out of needing work in the implementation.

Yeah, on platforms where the C++ member function calling convention is identical to the one of regular C functions, it's a no-op. On i386 windows, where the thiscall convention takes the this pointer in ecx instead of as a parameter on the stack, it's a couple instructions of difference - but it's all inlined into the caller in UnwindCursor::jumpto() anyway, so there's no extra stack frame inbetween.

I think that the confusing part is that it is not that they are C functions, they are still assembly, but with C calling convention. I think that you should state that in the description as it currently reads as though they are written in C, which they are not.

Right, there may need to be a thunk for some platforms but if the compiler does treat the call to the symbol as a tail call, that is fine.

Can you share the reasoning for building libunwind with MSVC? In particular, the calling convention isn't identical to the Unix calling convention, so you will need to use the Microsoft unwinder to get interoperability with MS ABI.

I think that the confusing part is that it is not that they are C functions, they are still assembly, but with C calling convention. I think that you should state that in the description as it currently reads as though they are written in C, which they are not.

Ah, I see. Rephrasing:

[libunwind] Convert the register restore functions to C calling convention and name mangling

Currently, the assembly functions for restoring register state have been direct implementations of the Registers_*::jumpto() method (contrary to the functions for saving register state, which are implementations of the extern C function __unw_getcontext). This has included having the assembly function name match the C++ mangling of that method name (and having the function match the C++ member function calling convention). To simplify the interface of the assembly implementations, make the functions have C calling conventions and name mangling.

This fixes building the library in with a MSVC C++ ABI with clang-cl, which uses a significantly different method name mangling scheme. (The library might not be of much use as C++ exception unwinder in such an environment, but the libunwind.h interface for stepwise unwinding still is usable, as is the _Unwind_Backtrace function.)

Right, there may need to be a thunk for some platforms but if the compiler does treat the call to the symbol as a tail call, that is fine.

Can you share the reasoning for building libunwind with MSVC? In particular, the calling convention isn't identical to the Unix calling convention, so you will need to use the Microsoft unwinder to get interoperability with MS ABI.

So for the actual calling convention, the existing assembly functions already have ifdefs so they work with the windows calling conventions for mingw.

Then for actual unwinding; libunwind, if operating in SEH mode, does call the Microsoft unwinder APIs, so it works in such an environment (just like it does in SEH mode for mingw). Most of the APIs from unwind.h doesn't make much sense to use there though (e.g. _Unwind_RaiseException and the related ones), but e.g. _Unwind_Backtrace should work just fine (just giving a callback for each stack frame), and the lower level libunwind.h APIs (unw_getcontext, unw_step, unw_get_reg etc) should work just fine.

If writing code from scratch for use on Windows, there'd be little point in using these instead of just calling the Microsoft unwinding APIs directly, but if porting user space code that already is calling these APIs on other platforms, it could make sense to use libunwind. That's the case that was posted on the llvm-dev list a few days ago.

compnerd accepted this revision.Aug 21 2020, 8:51 AM

Sounds good to me; LGTM with the rephrased commit message.

This revision is now accepted and ready to land.Aug 21 2020, 8:51 AM
This revision was landed with ongoing or failed builds.Aug 26 2020, 4:33 AM
This revision was automatically updated to reflect the committed changes.