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

14031 diff mbox series

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@ideasonboard.com>
---
Changes since v1:

- Use __noinline__ attribute for backtraceTrace() and unwindTrace()
---
 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(-)

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@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?

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

> ---
>  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.

Reviewed-by: Kieran Bingham <kieran.bingham@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
>

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@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@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