[libcamera-devel] [PATCH] libcamera: log: Destroy LogCategory instances in a controlled way

[Laurent Pinchart]

Hi Hiro,

On Mon, May 24, 2021 at 02:19:03PM +0900, Hirokazu Honda wrote:
> On Sun, May 23, 2021 at 9:04 AM Laurent Pinchart wrote:
> > The LogCategory instances are constructed on first use as static
> > variables in accessor functions, following the Meyers singleton pattern.
> > As a result, their destruction order is not guaranteed. This can cause
> > issues as the global Logger object, constructed in a similar fashion, is
> > accessed from the LogCategory destructor and may be destroyed first.
> >
> > To fix this, keep the same singleton pattern, but allocate the
> > LogCategory instances dynamically. As they get registered with the
> > global Logger instance, we can destroy them in the Logger destructor.
> >
> > This only avoids destruction order issues between LogCategory and
> > Logger, and doesn't address yet the fact that LOG() calls from
> > destructors of global objects may access an already destroyed Logger.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/internal/log.h |  5 ++---
> >  src/libcamera/log.cpp            | 32 +++++++++++---------------------
> >  2 files changed, 13 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/libcamera/internal/log.h
> > b/include/libcamera/internal/log.h
> > index b66bf55bc57d..1032bdd93e7c 100644
> > --- a/include/libcamera/internal/log.h
> > +++ b/include/libcamera/internal/log.h
> > @@ -29,7 +29,6 @@ class LogCategory
> >  {
> >  public:
> >         explicit LogCategory(const char *name);
> > -       ~LogCategory();
> >
> >         const char *name() const { return name_; }
> >         LogSeverity severity() const { return severity_; }
> > @@ -48,8 +47,8 @@ extern const LogCategory &_LOG_CATEGORY(name)();
> >  #define LOG_DEFINE_CATEGORY(name)                                      \
> >  const LogCategory &_LOG_CATEGORY(name)()                               \
> >  {                                                                      \
> > -       static LogCategory category(#name);                             \
> > -       return category;                                                \
> 
> I would like to have a comment that the created category will be deleted on
> Logger destruction?

Good point, I'll add that.

	/* The instance will be deleted by the Logger destructor. */

> With that, this change looks good to me.
> 
> Reviewed-by: Hirokazu Honda <hiroh at chormium.org>

Thank you.

> +       static LogCategory *category = new LogCategory(#name);          \
> > +       return *category;                                               \
> >  }
> >
> >  class LogMessage
> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> > index dd991647b9bc..74829a56916e 100644
> > --- a/src/libcamera/log.cpp
> > +++ b/src/libcamera/log.cpp
> > @@ -248,6 +248,8 @@ void LogOutput::writeStream(const std::string &str)
> >  class Logger
> >  {
> >  public:
> > +       ~Logger();
> > +
> >         static Logger *instance();
> >
> >         void write(const LogMessage &msg);
> > @@ -267,7 +269,6 @@ private:
> >
> >         friend LogCategory;
> >         void registerCategory(LogCategory *category);
> > -       void unregisterCategory(LogCategory *category);
> >
> >         std::unordered_set<LogCategory *> categories_;
> >         std::list<std::pair<std::string, LogSeverity>> levels_;
> > @@ -369,6 +370,12 @@ void logSetLevel(const char *category, const char
> > *level)
> >         Logger::instance()->logSetLevel(category, level);
> >  }
> >
> > +Logger::~Logger()
> > +{
> > +       for (LogCategory *category : categories_)
> > +               delete category;
> > +}
> > +
> >  /**
> >   * \brief Retrieve the logger instance
> >   *
> > @@ -665,18 +672,6 @@ void Logger::registerCategory(LogCategory *category)
> 
>         }
> >  }
> >
> > -/**
> > - * \brief Unregister a log category from the logger
> > - * \param[in] category The log category
> > - *
> > - * If the \a category hasn't been registered with the logger this function
> > - * performs no operation.
> > - */
> > -void Logger::unregisterCategory(LogCategory *category)
> > -{
> > -       categories_.erase(category);
> > -}
> > -
> >  /**
> >   * \enum LogSeverity
> >   * Log message severity
> > @@ -711,11 +706,6 @@ LogCategory::LogCategory(const char *name)
> >         Logger::instance()->registerCategory(this);
> >  }
> >
> > -LogCategory::~LogCategory()
> > -{
> > -       Logger::instance()->unregisterCategory(this);
> > -}
> > -
> >  /**
> >   * \fn LogCategory::name()
> >   * \brief Retrieve the log category name
> > @@ -746,12 +736,12 @@ void LogCategory::setSeverity(LogSeverity severity)
> >   * The default log category is named "default" and is used by the LOG() macro
> >   * when no log category is specified.
> >   *
> > - * \return A pointer to the default log category
> > + * \return A reference to the default log category
> >   */
> >  const LogCategory &LogCategory::defaultCategory()
> >  {
> > -       static const LogCategory category("default");
> > -       return category;
> > +       static const LogCategory *category = new LogCategory("default");
> > +       return *category;
> >  }
> >
> >  /**

-- 
Regards,

Laurent Pinchart