| Message ID | 20200221163130.4791-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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() Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > + > 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],
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@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@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], >
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@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@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],
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; +} + 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],
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@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(-)