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

[Laurent Pinchart]

Hi Nicolas,

On Tue, Sep 28, 2021 at 10:27:18AM -0400, Nicolas Dufresne wrote:
> Le mardi 28 septembre 2021 à 05:35 +0300, Laurent Pinchart a écrit :
> > On Mon, Sep 27, 2021 at 10:04:51AM -0400, Nicolas Dufresne wrote:
> > > Le vendredi 24 septembre 2021 à 13:23 +0300, Laurent Pinchart a écrit :
> > > > 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>
> > > > ---
> > > >  include/libcamera/base/backtrace.h |  3 ++
> > > >  src/libcamera/base/backtrace.cpp   | 61 ++++++++++++++++++++++++++++--
> > > >  src/libcamera/base/meson.build     |  6 +++
> > > >  3 files changed, 67 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 011f2e428d5d..40fa60d0f9bf 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
> > > > + * implementation.
> > > > + */
> > > > +#define UNW_LOCAL_ONLY
> > > > +#include <libunwind.h>
> > > > +#endif
> > > > +
> > > >  #include <sstream>
> > > >  
> > > >  #include <libcamera/base/span.h>
> > > > @@ -146,6 +155,15 @@ 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();
> > > > +}
> > > > +
> > > > +bool Backtrace::backtraceTrace()
> > > >  {
> > > >  #if HAVE_BACKTRACE
> > > >  	backtrace_.resize(32);
> > > > @@ -153,10 +171,44 @@ 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
> > > > +}
> > > > +
> > > > +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 +233,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;
> > > 
> > > Is this value specific to unwind ? Or perhaps you simply had it wrong in the
> > > original ? Perhaps you could fix the original or comment ?
> > 
> > It was 1 before because backtrace() was called directly in
> > Backtrace::Backtrace(), while now there are two helper functions
> > (backtraceTrace() and unwindTrace()) called by the constructor.
> 
> Thank you, I had totally missed the point of why we had to skip. Now said, its
> sounds all very obvious. Now I have a new concern, what if the compiler auto-
> inline some of this ? Do we need to add something to prevent this, or is it all
> safe ?

You ask very good questions :-) I've thought about it too, but didn't
reach an obvious conclusion. We could use the __noinline__ attribute to
ensure that the functions don't get inlined:

noinline

    This function attribute prevents a function from being considered
    for inlining. If the function does not have side effects, there are
    optimizations other than inlining that cause function calls to be
    optimized away, although the function call is live. To keep such
    calls from being optimized away, put

    asm ("");

    (see Extended Asm) in the called function, to serve as a special
    side effect.

We'll lose possible optimizations, but laybe it's not a big deal here ?

The other option is to force inlining of those functions, but the
"inline" keyword is a hint only. We could try the __always_inline__
attribute, I'm not sure if it works for member functions though, and it
may not be as clearcut as its name implies:

always_inline

    Generally, functions are not inlined unless optimization is
    specified. For functions declared inline, this attribute inlines the
    function independent of any restrictions that otherwise apply to
    inlining. Failure to inline such a function is diagnosed as an
    error. Note that if such a function is called indirectly the
    compiler may or may not inline it depending on optimization level
    and a failure to inline an indirect call may or may not be
    diagnosed.

I'm tempted to go for noinline.

> > > >  
> > > >  	if (backtrace_.size() <= skipLevels)
> > > >  		return std::string();
> > > > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> > > > index 1fa894cf1896..8d14a65648fc 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