[libcamera-devel] ipa: raspberrypi: Fix crash under LTO

18378 diff mbox series

From: Dave Jones <dave.jones@canonical.com>

When compiled with LTO (the default on Ubuntu), the global static
objects camHelpers and algorithms cause a crash in raspberrypi_ipa_proxy
at runtime as they're not allocated by the time the registration
routines execute.

This is a fairly crude fix which just converts the global static objects
into local static objects inside an equivalently named function.

Signed-off-by: Dave Jones <dave.jones@canonical.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Tested-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp           | 16 +++++++++++++---
 src/ipa/raspberrypi/controller/algorithm.cpp | 19 +++++++++++++++----
 2 files changed, 28 insertions(+), 7 deletions(-)

Hi Naush and Dave,

Thank you for the patch.

On Fri, Mar 10, 2023 at 09:51:26AM +0000, Naushir Patuck via libcamera-devel wrote:
> From: Dave Jones <dave.jones@canonical.com>
> 
> When compiled with LTO (the default on Ubuntu), the global static
> objects camHelpers and algorithms cause a crash in raspberrypi_ipa_proxy
> at runtime as they're not allocated by the time the registration
> routines execute.
> 
> This is a fairly crude fix which just converts the global static objects
> into local static objects inside an equivalently named function.
> 
> Signed-off-by: Dave Jones <dave.jones@canonical.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp           | 16 +++++++++++++---
>  src/ipa/raspberrypi/controller/algorithm.cpp | 19 +++++++++++++++----
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index d90ac1deda47..efe453dc6135 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <limits>
>  #include <map>
> +#include <memory>
>  #include <string.h>
>  
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -25,7 +26,16 @@ namespace libcamera {
>  LOG_DECLARE_CATEGORY(IPARPI)
>  }
>  
> -static std::map<std::string, CamHelperCreateFunc> camHelpers;
> +namespace {
> +
> +std::map<std::string, CamHelperCreateFunc> &camHelpers()
> +{
> +	static std::unique_ptr<std::map<std::string, CamHelperCreateFunc>> obj =
> +		std::make_unique<std::map<std::string, CamHelperCreateFunc>>();
> +	return *obj;

I think you could simply write

	static std::map<std::string, CamHelperCreateFunc> helpers;
	return helpers;

> +}
> +
> +} /* namespace */
>  
>  CamHelper *CamHelper::create(std::string const &camName)
>  {
> @@ -33,7 +43,7 @@ CamHelper *CamHelper::create(std::string const &camName)
>  	 * CamHelpers get registered by static RegisterCamHelper
>  	 * initialisers.
>  	 */
> -	for (auto &p : camHelpers) {
> +	for (auto &p : camHelpers()) {
>  		if (camName.find(p.first) != std::string::npos)
>  			return p.second();
>  	}
> @@ -253,5 +263,5 @@ void CamHelper::populateMetadata([[maybe_unused]] const MdParser::RegisterMap &r
>  RegisterCamHelper::RegisterCamHelper(char const *camName,
>  				     CamHelperCreateFunc createFunc)
>  {
> -	camHelpers[std::string(camName)] = createFunc;
> +	camHelpers()[std::string(camName)] = createFunc;
>  }
> diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp
> index 6d91ee292bd1..2650f98bee5b 100644
> --- a/src/ipa/raspberrypi/controller/algorithm.cpp
> +++ b/src/ipa/raspberrypi/controller/algorithm.cpp
> @@ -4,9 +4,10 @@
>   *
>   * algorithm.cpp - ISP control algorithms
>   */
> -

Unrelated and unneeded change.

>  #include "algorithm.h"
>  
> +#include <memory>
> +
>  using namespace RPiController;
>  
>  int Algorithm::read([[maybe_unused]] const libcamera::YamlObject &params)
> @@ -34,14 +35,24 @@ void Algorithm::process([[maybe_unused]] StatisticsPtr &stats,
>  
>  /* For registering algorithms with the system: */
>  
> -static std::map<std::string, AlgoCreateFunc> algorithms;
> +namespace {
> +
> +static std::map<std::string, AlgoCreateFunc> &algorithms()
> +{
> +	static std::unique_ptr<std::map<std::string, AlgoCreateFunc>> obj =
> +		std::make_unique<std::map<std::string, AlgoCreateFunc>>();
> +	return *obj;

Same here.

> +}
> +
> +} /* namespace */
> +
>  std::map<std::string, AlgoCreateFunc> const &RPiController::getAlgorithms()
>  {
> -	return algorithms;
> +	return algorithms();
>  }
>  
>  RegisterAlgorithm::RegisterAlgorithm(char const *name,
>  				     AlgoCreateFunc createFunc)
>  {
> -	algorithms[std::string(name)] = createFunc;
> +	algorithms()[std::string(name)] = createFunc;
>  }

Hi Laurent,

On Fri, Mar 10, 2023 at 12:40:33PM +0200, Laurent Pinchart wrote:
[snip]
>> -static std::map<std::string, CamHelperCreateFunc> camHelpers;
>> +namespace {
>> +
>> +std::map<std::string, CamHelperCreateFunc> &camHelpers()
>> +{
>> +	static std::unique_ptr<std::map<std::string, CamHelperCreateFunc>> obj =
>> +		std::make_unique<std::map<std::string, CamHelperCreateFunc>>();
>> +	return *obj;
>
>I think you could simply write
>
>	static std::map<std::string, CamHelperCreateFunc> helpers;
>	return helpers;

That would likely be fine, but I thought I'd just note that in my 
original patch [1] I'd actually used "new" with a pointer instead of a 
static object quite deliberately. As my C++ skills are caked in the dust 
of grime of numerous years of neglect, I'd consulted the C++ FAQ and 
noted that using a static pointer opens the possibility of a crash on 
deinit [2] (I'm not certain it *would* crash, but I figured I'd take the 
easy/safe way out and leave the OS to deal with the memory) by using the 
solution posited at [3].

[1]: https://github.com/raspberrypi/libcamera/pull/46/files

[2]: https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2

[3]: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use

That *might* be preferable, though I admit it looks ... somewhat ugly.

Cheers,

Dave Jones.

Hi Dave,

On Fri, Mar 10, 2023 at 11:58:53AM +0000, Dave Jones wrote:
> On Fri, Mar 10, 2023 at 12:40:33PM +0200, Laurent Pinchart wrote:
> [snip]
> >> -static std::map<std::string, CamHelperCreateFunc> camHelpers;
> >> +namespace {
> >> +
> >> +std::map<std::string, CamHelperCreateFunc> &camHelpers()
> >> +{
> >> +	static std::unique_ptr<std::map<std::string, CamHelperCreateFunc>> obj =
> >> +		std::make_unique<std::map<std::string, CamHelperCreateFunc>>();
> >> +	return *obj;
> >
> > I think you could simply write
> >
> >	static std::map<std::string, CamHelperCreateFunc> helpers;
> >	return helpers;
> 
> That would likely be fine, but I thought I'd just note that in my 
> original patch [1] I'd actually used "new" with a pointer instead of a 
> static object quite deliberately. As my C++ skills are caked in the dust 
> of grime of numerous years of neglect, I'd consulted the C++ FAQ and 
> noted that using a static pointer opens the possibility of a crash on 
> deinit [2] (I'm not certain it *would* crash, but I figured I'd take the 
> easy/safe way out and leave the OS to deal with the memory) by using the 
> solution posited at [3].
> 
> [1]: https://github.com/raspberrypi/libcamera/pull/46/files
> 
> [2]: https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> 
> [3]: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
> 
> That *might* be preferable, though I admit it looks ... somewhat ugly.

Unless I'm mistaken, given that you're using a unique pointer, the
object will get deleted at deinit time. It should thus be functionally
equivalent to the static variable in v2.

We *could* allocate the objects dynamically without std::unique_ptr to
avoid deinit issues, but we would then leak memory. That won't cause any
issue in production, but for development it's nice if we can run
libcamera applications under valgrind and expect no leaks.

Hi all,


On Fri, 10 Mar 2023 at 13:41, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Dave,
>
> On Fri, Mar 10, 2023 at 11:58:53AM +0000, Dave Jones wrote:
> > On Fri, Mar 10, 2023 at 12:40:33PM +0200, Laurent Pinchart wrote:
> > [snip]
> > >> -static std::map<std::string, CamHelperCreateFunc> camHelpers;
> > >> +namespace {
> > >> +
> > >> +std::map<std::string, CamHelperCreateFunc> &camHelpers()
> > >> +{
> > >> +  static std::unique_ptr<std::map<std::string, CamHelperCreateFunc>>
> obj =
> > >> +          std::make_unique<std::map<std::string,
> CamHelperCreateFunc>>();
> > >> +  return *obj;
> > >
> > > I think you could simply write
> > >
> > >     static std::map<std::string, CamHelperCreateFunc> helpers;
> > >     return helpers;
> >
> > That would likely be fine, but I thought I'd just note that in my
> > original patch [1] I'd actually used "new" with a pointer instead of a
> > static object quite deliberately. As my C++ skills are caked in the dust
> > of grime of numerous years of neglect, I'd consulted the C++ FAQ and
> > noted that using a static pointer opens the possibility of a crash on
> > deinit [2] (I'm not certain it *would* crash, but I figured I'd take the
> > easy/safe way out and leave the OS to deal with the memory) by using the
> > solution posited at [3].
> >
> > [1]: https://github.com/raspberrypi/libcamera/pull/46/files
> >
> > [2]: https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> >
> > [3]: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
> >
> > That *might* be preferable, though I admit it looks ... somewhat ugly.
>
> Unless I'm mistaken, given that you're using a unique pointer, the
> object will get deleted at deinit time. It should thus be functionally
> equivalent to the static variable in v2.
>
> We *could* allocate the objects dynamically without std::unique_ptr to
> avoid deinit issues, but we would then leak memory. That won't cause any
> issue in production, but for development it's nice if we can run
> libcamera applications under valgrind and expect no leaks.
>

The unique_ptr was my addition to avoid leaks in debug.
I think using a static object like in v2 ought to be good for this fix.

Regards,
Naush



>
> --
> Regards,
>
> Laurent Pinchart
>

On Fri, Mar 10, 2023 at 01:43:44PM +0000, Naushir Patuck wrote:
>On Fri, 10 Mar 2023 at 13:41, Laurent Pinchart <
>laurent.pinchart@ideasonboard.com> wrote:
[snip]
>> Unless I'm mistaken, given that you're using a unique pointer, the
>> object will get deleted at deinit time. It should thus be functionally
>> equivalent to the static variable in v2.
>>
>> We *could* allocate the objects dynamically without std::unique_ptr to
>> avoid deinit issues, but we would then leak memory. That won't cause any
>> issue in production, but for development it's nice if we can run
>> libcamera applications under valgrind and expect no leaks.

The valgrind point is certainly a powerful argument in favour of using a 
static object here.

>The unique_ptr was my addition to avoid leaks in debug.
>I think using a static object like in v2 ought to be good for this fix.

Yes, it's a much nicer "read" than repeating all the map gubbins.