[libcamera-devel] [PATCH v1 1/4] libcamera: base: Add Backtrace class

[Laurent Pinchart]

Hi Nicolas,

On Mon, Sep 27, 2021 at 09:41:46AM -0400, Nicolas Dufresne wrote:
> Le vendredi 24 septembre 2021 à 13:23 +0300, Laurent Pinchart a écrit :
> > Create a new class to abstract generation and access to call stack
> > backtraces. The current implementation depends on the glibc backtrace()
> > implementation and is copied from the logger. Future development will
> > bring support for libunwind, transparently for the users of the class.
> > 
> > The logger backtrace implementation is dropped, replaced by usage of the
> > new Backtrace class.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/base/backtrace.h |  34 +++++++++
> >  include/libcamera/base/meson.build |   1 +
> >  meson.build                        |   4 --
> >  src/libcamera/base/backtrace.cpp   | 107 +++++++++++++++++++++++++++++
> >  src/libcamera/base/log.cpp         |  25 ++-----
> >  src/libcamera/base/meson.build     |   5 ++
> >  6 files changed, 153 insertions(+), 23 deletions(-)
> >  create mode 100644 include/libcamera/base/backtrace.h
> >  create mode 100644 src/libcamera/base/backtrace.cpp
> > 
> > diff --git a/include/libcamera/base/backtrace.h b/include/libcamera/base/backtrace.h
> > new file mode 100644
> > index 000000000000..aefc76defa83
> > --- /dev/null
> > +++ b/include/libcamera/base/backtrace.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * backtrace.h - Call stack backtraces
> > + */
> > +#ifndef __LIBCAMERA_BASE_BACKTRACE_H__
> > +#define __LIBCAMERA_BASE_BACKTRACE_H__
> 
> In the context of this being modern code base, have you considered using this ?
> 
>   #pragma once

Yes :-) I think we should do a tree-wide change, just haven't had the
time to do so. Is there any drawback btw ?

> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <libcamera/base/private.h>
> > +
> > +#include <libcamera/base/class.h>
> > +
> > +namespace libcamera {
> > +
> > +class Backtrace
> > +{
> > +public:
> > +	Backtrace();
> > +
> > +	std::string toString(unsigned int skipLevels = 0) const;
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(Backtrace)
> > +
> > +	std::vector<void *> backtrace_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_BASE_BACKTRACE_H__ */
> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> > index 9feb4b9346d5..525aba9d2919 100644
> > --- a/include/libcamera/base/meson.build
> > +++ b/include/libcamera/base/meson.build
> > @@ -3,6 +3,7 @@
> >  libcamera_base_include_dir = libcamera_include_dir / 'base'
> >  
> >  libcamera_base_headers = files([
> > +    'backtrace.h',
> >      'bound_method.h',
> >      'class.h',
> >      'event_dispatcher.h',
> > diff --git a/meson.build b/meson.build
> > index a49c484fe64e..dfed01ba26bc 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -29,10 +29,6 @@ cc = meson.get_compiler('c')
> >  cxx = meson.get_compiler('cpp')
> >  config_h = configuration_data()
> >  
> > -if cc.has_header_symbol('execinfo.h', 'backtrace')
> > -    config_h.set('HAVE_BACKTRACE', 1)
> > -endif
> > -
> >  if cc.has_header_symbol('unistd.h', 'issetugid')
> >      config_h.set('HAVE_ISSETUGID', 1)
> >  endif
> > diff --git a/src/libcamera/base/backtrace.cpp b/src/libcamera/base/backtrace.cpp
> > new file mode 100644
> > index 000000000000..913f7ba71b03
> > --- /dev/null
> > +++ b/src/libcamera/base/backtrace.cpp
> > @@ -0,0 +1,107 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * backtrace.h - Call stack backtraces
> > + */
> > +
> > +#include <libcamera/base/backtrace.h>
> > +
> > +#if HAVE_BACKTRACE
> > +#include <execinfo.h>
> > +#include <stdlib.h>
> > +#endif
> > +
> > +#include <sstream>
> > +
> > +#include <libcamera/base/span.h>
> > +
> > +/**
> > + * \file backtrace.h
> > + * \brief Generate call stack backtraces
>
>                     _s

I meant the imperative mood :-) I don't mind changing it though.

> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Backtrace
> > + * \brief Representation of a call stack backtrace
> > + *
> > + * The Backtrace class represents a function call stack. Constructing an
> > + * instance captures the call stack at the point the instance is constructed.
> > + * The instance can later be used to access the call stack and generate a
>                                                                          _s

I meant "to access the call stack and [to] generate ...". I'll add the
"to".

> > + * human-readable representation with the toString() function.
> > + *
> > + * Depending on the platform, different backends can be used to generate the
> > + * backtrace. The Backtrace class provides a best effort to capture accurate
> > + * backtraces, but doesn't offer any guarantee of a particular backtrace format.
> > + */
> > +
> > +/**
> > + * \brief Construct a backtrace
> > + *
> > + * The backtrace captures the call stack at the point where it is constructed.
> > + * It can later be converted to a string with toString().
> > + */
> > +Backtrace::Backtrace()
> > +{
> > +#if HAVE_BACKTRACE
> > +	backtrace_.resize(32);
> > +
> > +	int num_entries = backtrace(backtrace_.data(), backtrace_.size());
> > +	if (num_entries < 0) {
> > +		backtrace_.clear();
> > +		return;
> > +	}
> > +
> > +	backtrace_.resize(num_entries);
> > +#endif
> > +}
> > +
> > +/**
> > + * \brief Convert a backtrace to a string representation
>                    _s

We use the imperative mood for the \brief statements.

> > + * \param[in] skipLevels Number of initial levels to skip in the backtrace
> > + *
> > + * The string representation of the backtrace is a multi-line string, with one
> > + * line per call stack entry. The format of the entries isn't specified and is
> > + * platform-dependent.
> > + *
> > + * The \a skipLevels parameter indicates how many initial entries to skip from
> > + * the backtrace. This can be used to hide functions that wrap the construction
> > + * of the Backtrace instance from the call stack. The Backtrace constructor
> > + * itself is automatically skipped and never shown in the backtrace.
> > + *
> > + * If backtrace generation fails for any reason (usually because the platform
> > + * doesn't support this feature), an empty string is returned.
> > + *
> > + * \return A string representation of the backtrace, or an empty string if
> > + * backtrace generation isn't possible
> 
> Just an alternative suggestion for when the backtrace is not possible, perhaps
> we could return a string that let user know the backtrace could be be made.
> Otherwise, all callers will have to check for empty string in order to give user
> feedback.

I was thinking about it, but then I thought it may be better not to
assume in this class how a caller may want to use it when backtrace
generation isn't possible. It also allows the caller to check if the
backtrace was successfully generated, which could be useful.

> > + */
> > +std::string Backtrace::toString(unsigned int skipLevels) const
> > +{
> > +	/* Skip the first entry, corresponding to the Backtrace construction. */
> > +	skipLevels += 1;
> > +
> > +	if (backtrace_.size() <= skipLevels)
> > +		return std::string();
> > +
> > +#if HAVE_BACKTRACE
> > +	Span<void *const> trace{ backtrace_ };
> > +	trace = trace.subspan(skipLevels);
> > +
> > +	char **strings = backtrace_symbols(trace.data(), trace.size());
> > +	if (strings) {
> > +		std::ostringstream msg;
> > +
> > +		for (unsigned int i = 0; i < trace.size(); ++i)
> 
> There is no direct way of enumerating that one ? (direct in the sense no index
> required)

backtrace_symbols() returns an array that contains the number of
elements passed as the second argument, and there's no terminator.

> > +			msg << strings[i] << std::endl;
> > +
> > +		free(strings);
> 
> If you only we had smart pointers like glib does (g_autofree) ;-P

I'll blame backtrace_symbols() for not using C++ ;-) Jokes aside,
"unsafe" code is expected when interfacing with such APIs.

> > +		return msg.str();
> > +	}
> > +#endif
> > +
> > +	return std::string();
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index a3e3f9ea2712..272c440589d4 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -8,9 +8,6 @@
> >  #include <libcamera/base/log.h>
> >  
> >  #include <array>
> > -#if HAVE_BACKTRACE
> > -#include <execinfo.h>
> > -#endif
> >  #include <fstream>
> >  #include <iostream>
> >  #include <list>
> > @@ -23,6 +20,7 @@
> >  
> >  #include <libcamera/logging.h>
> >  
> > +#include <libcamera/base/backtrace.h>
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/utils.h>
> >  
> > @@ -418,31 +416,20 @@ void Logger::write(const LogMessage &msg)
> >   */
> >  void Logger::backtrace()
> >  {
> > -#if HAVE_BACKTRACE
> >  	std::shared_ptr<LogOutput> output = std::atomic_load(&output_);
> >  	if (!output)
> >  		return;
> >  
> > -	void *buffer[32];
> > -	int num_entries = ::backtrace(buffer, std::size(buffer));
> > -	char **strings = backtrace_symbols(buffer, num_entries);
> > -	if (!strings)
> > -		return;
> > -
> > -	std::ostringstream msg;
> > -	msg << "Backtrace:" << std::endl;
> > -
> >  	/*
> >  	 * Skip the first two entries that correspond to this function and
> >  	 * ~LogMessage().
> >  	 */
> > -	for (int i = 2; i < num_entries; ++i)
> > -		msg << strings[i] << std::endl;
> > +	std::string backtrace = Backtrace().toString(2);
> > +	if (backtrace.empty())
> > +		return;
> 
> As mention earlier, we have to do empty string check, and then this function
> does not even let user know that no backtrace was available. This might be
> confusing for leak tracers when the users explicitly ask for backtraces.

I agree a message would be good. I'll experiment with both options
(printing it here, or in Backtrace::toString()).

> >  
> > -	output->write(msg.str());
> > -
> > -	free(strings);
> > -#endif
> > +	output->write("Backtrace:\n");
> > +	output->write(backtrace);
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> > index 8e125744d823..85af01a19365 100644
> > --- a/src/libcamera/base/meson.build
> > +++ b/src/libcamera/base/meson.build
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> >  libcamera_base_sources = files([
> > +    'backtrace.cpp',
> >      'class.cpp',
> >      'bound_method.cpp',
> >      'event_dispatcher.cpp',
> > @@ -18,6 +19,10 @@ libcamera_base_sources = files([
> >      'utils.cpp',
> >  ])
> >  
> > +if cc.has_header_symbol('execinfo.h', 'backtrace')
> > +    config_h.set('HAVE_BACKTRACE', 1)
> > +endif
> > +
> >  libcamera_base_deps = [
> >      dependency('threads'),
> >      libatomic,

-- 
Regards,

Laurent Pinchart