Conversation
In case of this is frame of signal handler, the IP should be incremented, because the IP saved in the signal handler points to first non-executed instruction, while FDE/CIE expects IP to be after the first non-executed instruction.
|
|
| _LIBUNWIND_WEAK_ALIAS(__unw_resume, unw_resume) | ||
|
|
||
| /// ClickHouse: Convenient helper (added for jemalloc) | ||
| int unw_backtrace(void **buffer, int size) { |
There was a problem hiding this comment.
The question is - what prevents having this helper outside of this library in our code? Does it use private API?
There was a problem hiding this comment.
Doesn't use private API. IIUC:
- this function is present in the original libunwind, but not in llvm-libunwind,
- jemalloc uses it:
contrib/jemalloc/src/prof_sys.cincludes<libunwind.h>and calls unw_backtrace().
So we have to put it in libunwind.h, or patch jemalloc, or (if I'm reading the code correctly) use mallctl("experimental.hooks.prof_backtrace", nullptr, nullptr, &unw_backtrace, 8) to provide the function at runtime.
There was a problem hiding this comment.
jemalloc uses it: contrib/jemalloc/src/prof_sys.c includes <libunwind.h> and calls unw_backtrace().
It does not use unw_backtrace, since JEMALLOC_PROF_LIBUNWIND is not defined, JEMALLOC_PROF_LIBGCC is used - https://github.com/ClickHouse/ClickHouse/blob/9e9862066fcf5fae5342289c6134627b335126f2/contrib/jemalloc-cmake/CMakeLists.txt#L182-L187
Example of stacktrace - https://pastila.nl/?00d7b55b/044b1c710ec1b6f40a1142ba6d8555ab#1spY2b4S8szuWI9B3XSdRA== (by @antonio2368 )
There was a problem hiding this comment.
Thanks for pointing out! There are 2 more uses of unw_backtrace, both in our own code:
contrib/llvm-project/libcxx/include/exceptionsrc/Common/StackTrace.cpp
After removing these it builds. We should probably replace them both with_Unwind_Backtrace.
There was a problem hiding this comment.
Egh, now, it is used - ClickHouse/ClickHouse#66346
So I'm suggesting to leave it as-is for now, until all the callers will be fixed
8 participants
Did this:
Context: this submodule is a fork of a discontinued mirror of llvm-libunwind. This fork is now completely divorced from that mirror, and we're copying the libunwind directory from https://github.com/llvm/llvm-project instead.
Perhaps we should delete
contrib/libunwindand usecontrib/llvm-project/libunwindinstead. One obstacle is thatcontrib/llvm-projectis quite old, and updating it seems like a lot of work. And maybe we'll want to use different versions of libunwind vs llvm. We could "update"contrib/llvm-project/libunwindwithout updating the rest ofcontrib/llvm-project(by copying the directory, just like this PR does), that would be easy, but then it doesn't seem that different than what we're doing here. In any case, this PR doesn't conflict with any future efforts to migrate tocontrib/llvm-project/libunwind.