[cpu linux] Implement stack walking #1405
Conversation
|
Test failing due to dependency on linux_threads |
|
As with all the other PRs, you can test a combination https://github.com/bwrsandman/xenia/tree/linux |
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
9c2b3e6 to
87fc132
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
87fc132 to
a3f5b77
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
a3f5b77 to
8b8d932
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
8b8d932 to
5c20589
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
5c20589 to
963614c
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
963614c to
a5f5c0e
Compare
|
conflicts with #1317 because create the same file .gdbint, but with different content greetings |
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
a5f5c0e to
72f7c75
Compare
|
See bottom of the first comment of #1430 about that conflict. |
bwrsandman
force-pushed
the
linux_stack_walker
branch
2 times, most recently
from
b8d9076 to
41b9ebf
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
41b9ebf to
c0a307e
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
5 times, most recently
from
e788b9d to
18accf2
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
8 times, most recently
from
d55fcdd to
0387faa
Compare
|
It seems like the inclusion of libunwind causes travis to fail. I cannot replicate this on my system. On travis, the tests end up stalling. |
bwrsandman
force-pushed
the
linux_stack_walker
branch
3 times, most recently
from
99dc788 to
9dc782b
Compare
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
9dc782b to
5c3754a
Compare
Implement stack walking using libunwind and libiberty. These two libraries would be need dependencies. The libunwind library is quite common on a linux system and libiberty is part of the GNU compiler. Add tests for stack_walker.
bwrsandman
force-pushed
the
linux_stack_walker
branch
from
5c3754a to
de0d41c
Compare
| namespace xe { | ||
| namespace cpu { | ||
|
|
||
| const int CAPTURE_SIGNAL = SIGUSR1; |
There was a problem hiding this comment.
The code base of Xenia uses kCaptureSignal naming for constants. constexpr is possibly also preferable, or, even better, static constexpr in a class if that's applicable here, or kPosixCaptureSignal at least since this is the xe::cpu namespace common to different platforms.
| // with same ip and depth | ||
| template <> | ||
| struct hash<xe::cpu::PosixStackCapture> { | ||
| typedef xe::cpu::PosixStackCapture argument_t; |
There was a problem hiding this comment.
Xenia code base uses Argument naming for types (using/typedef, structures/classes, and template parameters).
| namespace xe { | ||
| namespace cpu { | ||
|
|
||
| #include <libiberty/demangle.h> |
There was a problem hiding this comment.
All #includes of header files should be in the beginning, unless it's an .inc file designed for inclusion in a specific location. Also, inclusion of third-party libraries must never be done inside custom namespaces (so <memory> doesn't create xe::cpu::std).
|
|
||
| #include <libiberty/demangle.h> | ||
| #include <memory> | ||
| std::string demangle(const char* mangled_name) { |
There was a problem hiding this comment.
CamelCase is used for functions (except for simple getters/setters in classes — which are also a debatable topic due to inconsistency). Also, this demangling is platform-specific, unless xe::cpu::demangle is common interface with platform-specific implementations chosen by including/excluding files into building, xe::cpu::demangle is an identifier that is likely to cause conflicts in the future.
| #include <libiberty/demangle.h> | ||
| #include <memory> | ||
| std::string demangle(const char* mangled_name) { | ||
| if (mangled_name == std::string()) { |
There was a problem hiding this comment.
For checking if an std::string is empty, .empty() can be used. In this case (a C string), [0] can be compared to 0.
| std::condition_variable cv; | ||
| volatile bool set; | ||
| unw_context_t context_; | ||
| } signal_handler_arg = {}; |
There was a problem hiding this comment.
Again, not named as platform-specific (as well as using snake_case and _t, which are inconsistent with type naming in Xenia).
| std::mutex mutex; | ||
| std::condition_variable cv; | ||
| volatile bool set; | ||
| unw_context_t context_; |
There was a problem hiding this comment.
Inconsistent member naming (one with an underscore, some without them). In a structure with directly accessible public fields, underscores are not needed, while they should be used in classes with getters/setters and private fields.
| auto capture = | ||
| PosixStackCapture(std::move(context), frame_offset, frame_count); | ||
|
|
||
| for (uint32_t i = 0; i < capture.cursors_.size(); ++i) { |
There was a problem hiding this comment.
size() of standard containers is a size_t, not a uint32_t.
| return false; | ||
| } | ||
| auto demangled_host_symbol_name = demangle(host_symbol_name.data()); | ||
| std::strncpy(frame.host_symbol.name, demangled_host_symbol_name.c_str(), |
There was a problem hiding this comment.
strncpy is dangerous (doesn't insert a null terminator in case of truncation). Overall, C strings should be avoided unless it's a hard requirement to use a preallocated number of bytes for a string.
| if (mangled_name == std::string()) { | ||
| return ""; | ||
| } | ||
| std::unique_ptr<char, decltype(&std::free)> ptr( |
There was a problem hiding this comment.
This usage of unique_ptr is highly confusing, and looks a lot like that's use-after-free until we notice that the pointer is being used for implicit std::string initialization rather than returned directly. An explicit std::free call should express the intention much more clearly, I think, and is short enough (std::free is safely defined as a no-op for nullptr). Though copy elision of the returned string is another question, I'm not sure how it will work with explicit additional code after the creation of the string — if this is done specifically to save copy elision, and there are no more straightforward options to force it, this approach to freeing should be fine.
4 participants
Implement stack walking using libunwind and libiberty.
These two libraries would be need dependencies.
The libunwind library is quite common on a linux system and libiberty is
part of the GNU compiler.
Add tests for stack_walker.
This, in combination with the other xenia linux fixes allows the backtrace to be visible at runtime:
