[libcamera-devel] [PATCH v1 2/4] libcamera: base: backtrace: Use libdw to provide symbolic names
Nicolas Dufresne
nicolas at ndufresne.ca
Tue Sep 28 16:25:03 CEST 2021
Le mardi 28 septembre 2021 à 05:33 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
>
> On Mon, Sep 27, 2021 at 10:01:15AM -0400, Nicolas Dufresne wrote:
> > Le vendredi 24 septembre 2021 à 13:23 +0300, Laurent Pinchart a écrit :
> > > libdw provides access to debugging information. This allows creating
> > > better stack trace entries, with file names and line numbers, but also
> > > with demangled symbols as the symbol name is available and can be passed
> > > to abi::__cxa_demangle().
> > >
> > > With libdw, the backtrace previously generated by backtrace_symbols()
> > >
> > > src/cam/../libcamera/libcamera.so(_ZN9libcamera14VimcCameraData4initEv+0xbd) [0x7f7dbb73222d]
> > > src/cam/../libcamera/libcamera.so(_ZN9libcamera19PipelineHandlerVimc5matchEPNS_16DeviceEnumeratorE+0x3e0) [0x7f7dbb731c40]
> > > src/cam/../libcamera/libcamera.so(_ZN9libcamera13CameraManager7Private22createPipelineHandlersEv+0x1a7) [0x7f7dbb5ea027]
> > > src/cam/../libcamera/libcamera.so(_ZN9libcamera13CameraManager7Private4initEv+0x98) [0x7f7dbb5e9dc8]
> > > src/cam/../libcamera/libcamera.so(_ZN9libcamera13CameraManager7Private3runEv+0x9f) [0x7f7dbb5e9c5f]
> > > src/cam/../libcamera/base/libcamera-base.so(_ZN9libcamera6Thread11startThreadEv+0xee) [0x7f7dbb3e95be]
> > > src/cam/../libcamera/base/libcamera-base.so(+0x4f9d7) [0x7f7dbb3ec9d7]
> > > src/cam/../libcamera/base/libcamera-base.so(+0x4f90e) [0x7f7dbb3ec90e]
> > > src/cam/../libcamera/base/libcamera-base.so(+0x4f2c2) [0x7f7dbb3ec2c2]
> > > /lib64/libpthread.so.0(+0x7e8e) [0x7f7dbab65e8e]
> > > /lib64/libc.so.6(clone+0x3f) [0x7f7dbb10b26f]
> > >
> > > becomes
> > >
> > > libcamera::VimcCameraData::init()+0xbd (src/libcamera/libcamera.so [0x00007f9de605b22d])
> > > libcamera::PipelineHandlerVimc::match(libcamera::DeviceEnumerator*)+0x3e0 (src/libcamera/libcamera.so [0x00007f9de605ac40])
> > > libcamera::CameraManager::Private::createPipelineHandlers()+0x1a7 (src/libcamera/libcamera.so [0x00007f9de5f13027])
> > > libcamera::CameraManager::Private::init()+0x98 (src/libcamera/libcamera.so [0x00007f9de5f12dc8])
> > > libcamera::CameraManager::Private::run()+0x9f (src/libcamera/libcamera.so [0x00007f9de5f12c5f])
> > > libcamera::Thread::startThread()+0xee (src/libcamera/base/libcamera-base.so [0x00007f9de5d125be])
> > > decltype(*(std::__1::forward<libcamera::Thread*>(fp0)).*fp()) std::__1::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*, void>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x77 (src/libcamera/base/libcamera-base.so [0x00007f9de5d159d7])
> > > void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*, 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*>&, std::__1::__tuple_indices<2ul>)+0x3e (src/libcamera/base/libcamera-base.so [0x00007f9de5d1590e])
> > > void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*> >(void*)+0x62 (src/libcamera/base/libcamera-base.so [0x00007f9de5d152c2])
> > > start_thread+0xde (/var/tmp/portage/sys-libs/glibc-2.33-r1/work/glibc-2.33/nptl/pthread_create.c:482)
> > > __clone+0x3f (../sysdeps/unix/sysv/linux/x86_64/clone.S:97)
> > >
> > > The stack entries related to libcamera are missing source file name and
> > > line information, which will be investigated separately, but this is
> > > still an improvement.
> > >
> > > Use libdw when available, falling back to backtrace_symbols() otherwise,
> > > or if libdw fails for any reason.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > src/libcamera/base/backtrace.cpp | 120 +++++++++++++++++++++++++++++++
> > > src/libcamera/base/meson.build | 7 ++
> > > 2 files changed, 127 insertions(+)
> > >
> > > diff --git a/src/libcamera/base/backtrace.cpp b/src/libcamera/base/backtrace.cpp
> > > index 913f7ba71b03..011f2e428d5d 100644
> > > --- a/src/libcamera/base/backtrace.cpp
> > > +++ b/src/libcamera/base/backtrace.cpp
> > > @@ -12,9 +12,16 @@
> > > #include <stdlib.h>
> > > #endif
> > >
> > > +#ifdef HAVE_DW
> > > +#include <cxxabi.h>
> > > +#include <elfutils/libdwfl.h>
> > > +#include <unistd.h>
> > > +#endif
> > > +
> > > #include <sstream>
> > >
> > > #include <libcamera/base/span.h>
> > > +#include <libcamera/base/utils.h>
> > >
> > > /**
> > > * \file backtrace.h
> > > @@ -23,6 +30,101 @@
> > >
> > > namespace libcamera {
> > >
> > > +namespace {
> > > +
> > > +#if HAVE_DW
> > > +class DwflParser
> > > +{
> > > +public:
> > > + DwflParser();
> > > + ~DwflParser();
> > > +
> > > + bool isValid() const { return valid_; }
> > > + std::string stackEntry(const void *ip);
> > > +
> > > +private:
> > > + Dwfl_Callbacks callbacks_;
> > > + Dwfl *dwfl_;
> > > + bool valid_;
> > > +};
> > > +
> > > +DwflParser::DwflParser()
> > > + : callbacks_({}), dwfl_(nullptr), valid_(false)
> > > +{
> > > + callbacks_.find_elf = dwfl_linux_proc_find_elf;
> > > + callbacks_.find_debuginfo = dwfl_standard_find_debuginfo;
> > > +
> > > + dwfl_ = dwfl_begin(&callbacks_);
> > > + if (!dwfl_)
> > > + return;
> > > +
> > > + int ret = dwfl_linux_proc_report(dwfl_, getpid());
> > > + if (ret)
> > > + return;
> > > +
> > > + ret = dwfl_report_end(dwfl_, nullptr, nullptr);
> > > + if (ret)
> > > + return;
> > > +
> > > + valid_ = true;
> > > +}
> > > +
> > > +DwflParser::~DwflParser()
> > > +{
> > > + if (dwfl_)
> > > + dwfl_end(dwfl_);
> > > +}
> > > +
> > > +std::string DwflParser::stackEntry(const void *ip)
> > > +{
> > > + Dwarf_Addr addr = reinterpret_cast<Dwarf_Addr>(ip);
> > > +
> > > + Dwfl_Module *module = dwfl_addrmodule(dwfl_, addr);
> > > + if (!module)
> > > + return std::string();
> > > +
> > > + std::ostringstream entry;
> > > +
> > > + GElf_Off offset;
> > > + GElf_Sym sym;
> > > + const char *symbol = dwfl_module_addrinfo(module, addr, &offset, &sym,
> > > + nullptr, nullptr, nullptr);
> > > + if (symbol) {
> > > + char *name = abi::__cxa_demangle(symbol, nullptr, nullptr, nullptr);
> >
> > A suggestion, and yes, you have to choose between memory safety and ugly code.
> >
> > std::unique_ptr<char[], decltype(free)*>{
> > abi::__cxa_demangle(symbol, nullptr, nullptr, nullptr),
> > free};
> >
> > This is needed as by default C++ will assume that delete[] is to be used rather
> > then free.
> >
> > > + entry << (name ? name : symbol) << "+0x" << std::hex << offset
> > > + << std::dec;
> > > + free(name);
> >
> > And this error prone free is not longer needed, since you have a smart pointer.
>
> I've actually written something similar to the above initially, but then
> considered that the free was close enough to the allocation, and the
> code unlikely enough to change in dangerous ways, that it wasn't worth
> it. I wouldn't mind changing it if a unique_ptr is preferred.
I agree. I think in the long term, we'll just add a helper that makes it less
ugly, and we will be more keen to use it.
>
> > > + } else {
> > > + entry << "??? [" << utils::hex(addr) << "]";
> > > + }
> > > +
> > > + entry << " (";
> > > +
> > > + Dwfl_Line *line = dwfl_module_getsrc(module, addr);
> > > + if (line) {
> > > + const char *filename;
> > > + int lineNumber = 0;
> > > +
> > > + filename = dwfl_lineinfo(line, &addr, &lineNumber, nullptr,
> > > + nullptr, nullptr);
> > > +
> > > + entry << (filename ? filename : "???") << ":" << lineNumber;
> > > + } else {
> > > + const char *filename = nullptr;
> > > +
> > > + dwfl_module_info(module, nullptr, nullptr, nullptr, nullptr,
> > > + nullptr, &filename, nullptr);
> > > +
> > > + entry << (filename ? filename : "???") << " [" << utils::hex(addr) << "]";
> > > + }
> > > +
> > > + entry << ")";
> > > + return entry.str();
> > > +}
> > > +#endif /* HAVE_DW */
> > > +
> > > +} /* namespace */
> > > +
> > > /**
> > > * \class Backtrace
> > > * \brief Representation of a call stack backtrace
> > > @@ -85,6 +187,24 @@ std::string Backtrace::toString(unsigned int skipLevels) const
> > > if (backtrace_.size() <= skipLevels)
> > > return std::string();
> > >
> > > +#if HAVE_DW
> > > + DwflParser dwfl;
> > > +
> > > + if (dwfl.isValid()) {
> > > + std::ostringstream msg;
> > > +
> > > + Span<void *const> trace{ backtrace_ };
> > > + for (const void *ip : trace.subspan(skipLevels)) {
> > > + if (ip)
> > > + msg << dwfl.stackEntry(ip) << std::endl;
> > > + else
> > > + msg << "???" << std::endl;
> > > + }
> > > +
> > > + return msg.str();
> > > + }
> > > +#endif
> > > +
> > > #if HAVE_BACKTRACE
> > > Span<void *const> trace{ backtrace_ };
> > > trace = trace.subspan(skipLevels);
> > > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> > > index 85af01a19365..1fa894cf1896 100644
> > > --- a/src/libcamera/base/meson.build
> > > +++ b/src/libcamera/base/meson.build
> > > @@ -19,13 +19,20 @@ libcamera_base_sources = files([
> > > 'utils.cpp',
> > > ])
> > >
> > > +libdw = cc.find_library('libdw', required : false)
> > > +
> > > if cc.has_header_symbol('execinfo.h', 'backtrace')
> > > config_h.set('HAVE_BACKTRACE', 1)
> > > endif
> > >
> > > +if libdw.found()
> > > + config_h.set('HAVE_DW', 1)
> > > +endif
> > > +
> > > libcamera_base_deps = [
> > > dependency('threads'),
> > > libatomic,
> > > + libdw,
> > > ]
> > >
> > > # Internal components must use the libcamera_base_private dependency to enable
>
More information about the libcamera-devel
mailing list