[libcamera-devel] libcamera: log: Document LOG() restrictions
diff mbox series

Message ID 20210521092214.1160-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit cc2139ec63c2d062b077c9c221917325c3f30f17
Headers show
Series
  • [libcamera-devel] libcamera: log: Document LOG() restrictions
Related show

Commit Message

Laurent Pinchart May 21, 2021, 9:22 a.m. UTC
The LOG() macro uses the global Logger and LogCategory objects
internally. This can result in crashes or other undefined behaviour when
logging from destructors of global objects, as the Logger and/or the
LogCategory instances may have been destroyed.

This isn't ideal and should eventually be fixed. For the time being,
document the restriction, and add a todo list item to fix the problem.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/log.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Hirokazu Honda May 21, 2021, 9:26 a.m. UTC | #1
Hi Laurent, thank you for the patch.

On Fri, May 21, 2021 at 6:22 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> The LOG() macro uses the global Logger and LogCategory objects
> internally. This can result in crashes or other undefined behaviour when
> logging from destructors of global objects, as the Logger and/or the
> LogCategory instances may have been destroyed.
>
> This isn't ideal and should eventually be fixed. For the time being,
> document the restriction, and add a todo list item to fix the problem.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>


> ---
>  src/libcamera/log.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 94175ab34535..985952249289 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -980,6 +980,12 @@ LogMessage _log(const LogCategory *category,
> LogSeverity severity,
>   *
>   * If the severity is set to Fatal, execution is aborted and the program
>   * terminates immediately after printing the message.
> + *
> + * \warning Logging from the destructor of a global object, either
> directly or
> + * indirectly, results in undefined behaviour.
> + *
> + * \todo Allow logging from destructors of global objects to the largest
> + * possible extent
>   */
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>
>
Paul Elder May 21, 2021, 10 a.m. UTC | #2
On Fri, May 21, 2021 at 12:22:14PM +0300, Laurent Pinchart wrote:
> The LOG() macro uses the global Logger and LogCategory objects
> internally. This can result in crashes or other undefined behaviour when
> logging from destructors of global objects, as the Logger and/or the
> LogCategory instances may have been destroyed.
> 
> This isn't ideal and should eventually be fixed. For the time being,
> document the restriction, and add a todo list item to fix the problem.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/log.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 94175ab34535..985952249289 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -980,6 +980,12 @@ LogMessage _log(const LogCategory *category, LogSeverity severity,
>   *
>   * If the severity is set to Fatal, execution is aborted and the program
>   * terminates immediately after printing the message.
> + *
> + * \warning Logging from the destructor of a global object, either directly or
> + * indirectly, results in undefined behaviour.
> + *
> + * \todo Allow logging from destructors of global objects to the largest
> + * possible extent
>   */
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>
Kieran Bingham May 21, 2021, 10:16 a.m. UTC | #3
Hi Laurent,

On 21/05/2021 10:22, Laurent Pinchart wrote:
> The LOG() macro uses the global Logger and LogCategory objects
> internally. This can result in crashes or other undefined behaviour when
> logging from destructors of global objects, as the Logger and/or the
> LogCategory instances may have been destroyed.
> 
> This isn't ideal and should eventually be fixed. For the time being,
> document the restriction, and add a todo list item to fix the problem.

Is there a way to catch this in code? Presumably it's something to do
with the global logging object being destructed, and then an action
occuring on that object?

Could we have something like the canary I proposed on the requests, so
that if any action on the log happens after it's destructed it fires a
more meaningful assert rather than a less parsable segfault/random failure?


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/log.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 94175ab34535..985952249289 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -980,6 +980,12 @@ LogMessage _log(const LogCategory *category, LogSeverity severity,
>   *
>   * If the severity is set to Fatal, execution is aborted and the program
>   * terminates immediately after printing the message.
> + *
> + * \warning Logging from the destructor of a global object, either directly or
> + * indirectly, results in undefined behaviour.
> + *
> + * \todo Allow logging from destructors of global objects to the largest
> + * possible extent

I guess there's no way to say "Please delete me last"?

Are there any tricks that can be played by trying to ensure the log is
the first thing constructed?

Anyway, documenting the current limitation is a good thing

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>   */
>  
>  /**
>
Laurent Pinchart May 21, 2021, 11:59 p.m. UTC | #4
Hi Kieran,

On Fri, May 21, 2021 at 11:16:40AM +0100, Kieran Bingham wrote:
> On 21/05/2021 10:22, Laurent Pinchart wrote:
> > The LOG() macro uses the global Logger and LogCategory objects
> > internally. This can result in crashes or other undefined behaviour when
> > logging from destructors of global objects, as the Logger and/or the
> > LogCategory instances may have been destroyed.
> > 
> > This isn't ideal and should eventually be fixed. For the time being,
> > document the restriction, and add a todo list item to fix the problem.
> 
> Is there a way to catch this in code? Presumably it's something to do
> with the global logging object being destructed, and then an action
> occuring on that object?

Correct.

> Could we have something like the canary I proposed on the requests, so
> that if any action on the log happens after it's destructed it fires a
> more meaningful assert rather than a less parsable segfault/random failure?

That should be possible, but I'd rather focus on addressing the todo
item below, to allow logging messages everywhere (expect probably from
the destructor of the Logger itself :-)).

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/log.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> > index 94175ab34535..985952249289 100644
> > --- a/src/libcamera/log.cpp
> > +++ b/src/libcamera/log.cpp
> > @@ -980,6 +980,12 @@ LogMessage _log(const LogCategory *category, LogSeverity severity,
> >   *
> >   * If the severity is set to Fatal, execution is aborted and the program
> >   * terminates immediately after printing the message.
> > + *
> > + * \warning Logging from the destructor of a global object, either directly or
> > + * indirectly, results in undefined behaviour.
> > + *
> > + * \todo Allow logging from destructors of global objects to the largest
> > + * possible extent
> 
> I guess there's no way to say "Please delete me last"?
> 
> Are there any tricks that can be played by trying to ensure the log is
> the first thing constructed?

I may start by looking at LogCategory first, to allocate them
dynamically and delete them when the Logger is deleted. When it comes to
the Logger itself, I'm not sure yet.

> Anyway, documenting the current limitation is a good thing
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >   */
> >  
> >  /**

Patch
diff mbox series

diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
index 94175ab34535..985952249289 100644
--- a/src/libcamera/log.cpp
+++ b/src/libcamera/log.cpp
@@ -980,6 +980,12 @@  LogMessage _log(const LogCategory *category, LogSeverity severity,
  *
  * If the severity is set to Fatal, execution is aborted and the program
  * terminates immediately after printing the message.
+ *
+ * \warning Logging from the destructor of a global object, either directly or
+ * indirectly, results in undefined behaviour.
+ *
+ * \todo Allow logging from destructors of global objects to the largest
+ * possible extent
  */
 
 /**