| Message ID | 20210924102323.26787-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 > + > +#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 > + */ > + > +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 > + * 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 > + * \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. > + */ > +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) > + msg << strings[i] << std::endl; > + > + free(strings); If you only we had smart pointers like glib does (g_autofree) ;-P > + 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. > > - 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,
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@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,
Le mardi 28 septembre 2021 à 05:31 +0300, Laurent Pinchart a écrit : > 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@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 ? Not that I know of, we held on this in GStreamer as MSVC compiler didn't support it, but this is has been well supported for years now. In general, this is 1 line instead of 3, it should have the same semantic, aka, it's once for the portion below. Perhaps you loose the ability to add code after the endif, but no one does that. > > > > + > > > +#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. Ah, well then it's fine, both ways. > > > > + */ > > > + > > > +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". Ack. > > > > + * 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. Ack. > > > > + * \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. Ack. > > > > + */ > > > +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. Ack. > > > > + 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()). Thanks for considering some of my suggestions. This is really good work and will be very handy in the future. > > > > > > > - 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, >
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__ + +#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 + */ + +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 + * 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 + * \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 + */ +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) + msg << strings[i] << std::endl; + + free(strings); + 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; - 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,
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@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