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

[Nicolas Dufresne]

Le mardi 28 septembre 2021 à 05:35 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> 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 ?

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