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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart