[libcamera-devel] [PATCH v4 5/6] libcamera: ipa_manager: Search for IPA libraries in build tree

[Laurent Pinchart]

Hi Kieran,

On Mon, Feb 24, 2020 at 09:44:39AM +0000, Kieran Bingham wrote:
> On 22/02/2020 10:42, Laurent Pinchart wrote:
> > On Fri, Feb 21, 2020 at 04:31:29PM +0000, Kieran Bingham wrote:
> >> When libcamera is built and tested (or used at all) before installing to
> >> the configured prefix path, it will be unable to locate the IPA
> >> binaries, or IPA binaries previously installed in the system paths may
> >> be incorrect to load.
> >>
> >> Utilise the build_rpath dynamic tag which is stripped out by meson at
> >> install time to determine at runtime if the library currently executing
> >> has been installed or not.
> >>
> >> When not installed and running from a build tree, identify the location
> >> of that tree by finding the path of the active libcamera.so itself, and
> >> from that point add a relative path to be able to load the most recently
> >> built IPA modules.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >> ---
> >> v2:
> >>  - Move get_runpath to ipa_manager.c : libcamera::elfRunPath()
> >>  - Minor fixes
> >>  - Squash elfRunPath() into this patch
> >>
> >> v3:
> >>  - Full rework. It's just all different :-)
> >>  - Maybe this one is going to cut it ...
> >>
> >> v4:
> >>  - rebase and cleanup
> >> ---
> >>  src/libcamera/ipa_manager.cpp | 52 ++++++++++++++++++++++++++++++++++-
> >>  src/libcamera/meson.build     |  6 ++++
> >>  2 files changed, 57 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >> index de99a0bc3ce1..a5f2707f6788 100644
> >> --- a/src/libcamera/ipa_manager.cpp
> >> +++ b/src/libcamera/ipa_manager.cpp
> >> @@ -9,6 +9,9 @@
> >>  
> >>  #include <algorithm>
> >>  #include <dirent.h>
> >> +#include <dlfcn.h>
> >> +#include <elf.h>
> >> +#include <link.h>
> >>  #include <string.h>
> >>  #include <sys/types.h>
> >>  
> >> @@ -24,6 +27,35 @@
> >>   * \brief Image Processing Algorithm module manager
> >>   */
> >>  
> >> +static bool isLibcameraInstalled()
> >> +{
> >> +	/* musl doesn't declare _DYNAMIC in link.h, declare it manually. */
> >> +	extern ElfW(Dyn) _DYNAMIC[];
> >> +
> >> +	/*
> >> +	 * DT_RUNPATH (DT_RPATH when the linker uses old dtags) is removed on
> >> +	 * install.
> >> +	 */
> >> +	for (const ElfW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> >> +		if (dyn->d_tag == DT_RUNPATH || dyn->d_tag == DT_RPATH)
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +static const char *libcameraPath()
> >> +{
> >> +	Dl_info info;
> >> +
> >> +	/* Look up our own symbol. */
> >> +	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
> >> +	if (ret == 0)
> >> +		return nullptr;
> >> +
> >> +	return info.dli_fname;
> > 
> > You're returning a pointer to a local stack variable. I'm surprised that
> > compilers don't warn us. The easiest fix is to turn the function into
> > 
> > static std::string libcameraPath()
> 
> Dl_info is a:
> 
> typedef struct
> {
>   const char *dli_fname;	/* File name of defining object.  */
>   void *dli_fbase;		/* Load address of that object.  */
>   const char *dli_sname;	/* Name of nearest symbol.  */
>   void *dli_saddr;		/* Exact value of nearest symbol.  */
> } Dl_info;
> 
> Thus I understood that info.dli_fname was a pointer to some constant
> linker load time memory location, as it certainly can't go out of scope
> just because the Dl_info structure goes out of scope ...

You're right, my bad.

> I'm fine returning a std::string though, as it's only used in string
> parsing anyway..

And the only user of libcameraPath() passes the return value to a
function that takes an std::string, so there's a conversion anyway :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Happy to convert to a std::string all the same and grab that tag...
> 
> >> +}
> >> +
> >>  namespace libcamera {
> >>  
> >>  LOG_DEFINE_CATEGORY(IPAManager)
> >> @@ -112,7 +144,25 @@ IPAManager::IPAManager()
> >>  				<< "No IPA found in '" << modulePaths << "'";
> >>  	}
> >>  
> >> -	/* Load IPAs from the installed system path. */
> >> +	/*
> >> +	 * When libcamera is used before it is installed, load IPAs from the
> >> +	 * same build directory as the libcamera library itself. This requires
> >> +	 * identifying the path of the libcamera.so, and referencing a relative
> >> +	 * path for the IPA from that point. We need to recurse one level of
> >> +	 * sub-directories to match the build tree.
> >> +	 */
> >> +	if (!isLibcameraInstalled()) {
> >> +		std::string ipaBuildPath = utils::dirname(libcameraPath()) + "/../ipa";
> >> +		constexpr int maxDepth = 1;
> >> +
> >> +		LOG(IPAManager, Info)
> >> +			<< "libcamera is not installed. Adding '"
> >> +			<< ipaBuildPath << "' to the IPA search path";
> >> +
> >> +		ipaCount += addDir(ipaBuildPath.c_str(), maxDepth);
> >> +	}
> >> +
> >> +	/* Finally try to load IPAs from the installed system path. */
> >>  	ipaCount += addDir(IPA_MODULE_DIR);
> >>  
> >>  	if (!ipaCount)
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 1e5b54b34078..88658ac563f7 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -107,11 +107,17 @@ if get_option('android')
> >>      libcamera_link_with += android_camera_metadata
> >>  endif
> >>  
> >> +# We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> >> +# The build_rpath is stripped at install time by meson, so we determine at
> >> +# runtime if the library is running from an installed location by checking
> >> +# for the presence or abscence of the dynamic tag.
> >> +
> >>  libcamera = shared_library('camera',
> >>                             libcamera_sources,
> >>                             install : true,
> >>                             link_with : libcamera_link_with,
> >>                             include_directories : includes,
> >> +                           build_rpath : '/',
> >>                             dependencies : libcamera_deps)
> >>  
> >>  libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_ipa_api, libcamera_h],

-- 
Regards,

Laurent Pinchart