[libcamera-devel] [PATCH v2 3/4] libcamera: base: backtrace: Use libunwind when available

[Laurent Pinchart]

Hi Kieran,

On Tue, Oct 12, 2021 at 11:52:10AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-10-03 23:36:05)
> > libunwind is an alternative to glibc's backtrace() to extract a
> > backtrace. Use it when available to extend backtrace support to more
> > platforms.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Use __noinline__ attribute for backtraceTrace() and unwindTrace()
> 
> Is this guaranteed ? Or do we need to do some checks at runtime?

As far as I understand, it's not fully guaranteed:

https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Function-Attributes.html#index-noinline-function-attribute

> Perhaps we can deal with that if we find out we need it.

That was exactly my reasoning :-)

> > ---
> >  include/libcamera/base/backtrace.h |  3 ++
> >  src/libcamera/base/backtrace.cpp   | 67 ++++++++++++++++++++++++++++--
> >  src/libcamera/base/meson.build     |  6 +++
> >  3 files changed, 73 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/libcamera/base/backtrace.h b/include/libcamera/base/backtrace.h
> > index aefc76defa83..58ccc14c8f81 100644
> > --- a/include/libcamera/base/backtrace.h
> > +++ b/include/libcamera/base/backtrace.h
> > @@ -26,6 +26,9 @@ public:
> >  private:
> >         LIBCAMERA_DISABLE_COPY(Backtrace)
> >  
> > +       bool backtraceTrace();
> > +       bool unwindTrace();
> > +
> >         std::vector<void *> backtrace_;
> >  };
> >  
> > diff --git a/src/libcamera/base/backtrace.cpp b/src/libcamera/base/backtrace.cpp
> > index 79e4a31f3d21..0aafc6a366c5 100644
> > --- a/src/libcamera/base/backtrace.cpp
> > +++ b/src/libcamera/base/backtrace.cpp
> > @@ -18,6 +18,15 @@
> >  #include <unistd.h>
> >  #endif
> >  
> > +#if HAVE_UNWIND
> > +/*
> > + * Disable support for remote unwinding to enable a more optimized
> 
> What is 'remote' unwinding? Not that it matters too much here.

It means unwinding a stack of another process (possibly running on a
different machine).

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > + * implementation.
> > + */
> > +#define UNW_LOCAL_ONLY
> > +#include <libunwind.h>
> > +#endif
> > +
> >  #include <sstream>
> >  
> >  #include <libcamera/base/span.h>
> > @@ -146,6 +155,20 @@ std::string DwflParser::stackEntry(const void *ip)
> >   * It can later be converted to a string with toString().
> >   */
> >  Backtrace::Backtrace()
> > +{
> > +       /* Try libunwind first and fall back to backtrace() if it fails. */
> > +       if (unwindTrace())
> > +               return;
> > +
> > +       backtraceTrace();
> > +}
> > +
> > +/*
> > + * Avoid inlining to make sure that the Backtrace constructor adds exactly two
> > + * calls to the stack, which are later skipped in toString().
> > + */
> > +__attribute__((__noinline__))
> > +bool Backtrace::backtraceTrace()
> >  {
> >  #if HAVE_BACKTRACE
> >         backtrace_.resize(32);
> > @@ -153,10 +176,45 @@ Backtrace::Backtrace()
> >         int num_entries = backtrace(backtrace_.data(), backtrace_.size());
> >         if (num_entries < 0) {
> >                 backtrace_.clear();
> > -               return;
> > +               return false;
> >         }
> >  
> >         backtrace_.resize(num_entries);
> > +
> > +       return true;
> > +#else
> > +       return false;
> > +#endif
> > +}
> > +
> > +__attribute__((__noinline__))
> > +bool Backtrace::unwindTrace()
> > +{
> > +#if HAVE_UNWIND
> > +       unw_context_t uc;
> > +       int ret = unw_getcontext(&uc);
> > +       if (ret)
> > +               return false;
> > +
> > +       unw_cursor_t cursor;
> > +       ret = unw_init_local(&cursor, &uc);
> > +       if (ret)
> > +               return false;
> > +
> > +       do {
> > +               unw_word_t ip;
> > +               ret = unw_get_reg(&cursor, UNW_REG_IP, &ip);
> > +               if (ret) {
> > +                       backtrace_.push_back(nullptr);
> > +                       continue;
> > +               }
> > +
> > +               backtrace_.push_back(reinterpret_cast<void *>(ip));
> > +       } while (unw_step(&cursor) > 0);
> > +
> > +       return true;
> > +#else
> > +       return false;
> >  #endif
> >  }
> >  
> > @@ -181,8 +239,11 @@ Backtrace::Backtrace()
> >   */
> >  std::string Backtrace::toString(unsigned int skipLevels) const
> >  {
> > -       /* Skip the first entry, corresponding to the Backtrace construction. */
> > -       skipLevels += 1;
> > +       /*
> > +        * Skip the first two entries, corresponding to the Backtrace
> > +        * construction.
> > +        */
> > +       skipLevels += 2;
> >  
> >         if (backtrace_.size() <= skipLevels)
> >                 return std::string();
> > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> > index 4c44b9f55011..05fed7acf561 100644
> > --- a/src/libcamera/base/meson.build
> > +++ b/src/libcamera/base/meson.build
> > @@ -20,6 +20,7 @@ libcamera_base_sources = files([
> >  ])
> >  
> >  libdw = cc.find_library('libdw', required : false)
> > +libunwind = cc.find_library('libunwind', required : false)
> >  
> >  if cc.has_header_symbol('execinfo.h', 'backtrace')
> >      config_h.set('HAVE_BACKTRACE', 1)
> > @@ -29,10 +30,15 @@ if libdw.found()
> >      config_h.set('HAVE_DW', 1)
> >  endif
> >  
> > +if libunwind.found()
> > +    config_h.set('HAVE_UNWIND', 1)
> > +endif
> > +
> >  libcamera_base_deps = [
> >      dependency('threads'),
> >      libatomic,
> >      libdw,
> > +    libunwind,
> >  ]
> >  
> >  # Internal components must use the libcamera_base_private dependency to enable

-- 
Regards,

Laurent Pinchart