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

[Kieran Bingham]

Hi Laurent,

On 22/02/2020 10:42, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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 ...

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


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Happy to convert to a std::string all the same and grab that tag...
--
Kieran


> 
>> +}
>> +
>>  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
--
Kieran