[libcamera-devel,1/5] libcamera: utils: Provide helper to get dynamic library runpath

Message ID 20200205130420.8736-2-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • Support loading IPAs from the build tree
Related show

Commit Message

Kieran Bingham Feb. 5, 2020, 1:04 p.m. UTC
Provide a helper that will identify the DT_RUNPATH string from the
ELF dynamic library string table entries.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/utils.h |  1 +
 src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Laurent Pinchart Feb. 6, 2020, 6:07 p.m. UTC | #1
Hi Kieran,

Thank you for the patch.

On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
> Provide a helper that will identify the DT_RUNPATH string from the
> ELF dynamic library string table entries.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/utils.h |  1 +
>  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index e467eb21c518..069b327bb436 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -33,6 +33,7 @@ namespace utils {
>  const char *basename(const char *path);
>  
>  char *secure_getenv(const char *name);
> +const char *get_runpath();
>  
>  template<class InputIt1, class InputIt2>
>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 4beffdab5eb6..e48cf2b6a48e 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -7,7 +7,9 @@
>  
>  #include "utils.h"
>  
> +#include <elf.h>
>  #include <iomanip>
> +#include <link.h>
>  #include <sstream>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
>  #endif
>  }
>  
> +/**
> + * \brief Obtain the runpath string from the dynamic symbol table

It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
(.dynsym).

> + * \returns a const char pointer to the runpath entry in the elf string table

s/returns/return/

Actually after checking the doxygen documentation \returns is an alias
for \return. Still, I think we should standardize on \return.

s/a const/A const/

Maybe s/elf/ELF/ ?

> + * or NULL if it is not found.

s/\.$//

> + */
> +const char *get_runpath()

We only use this function in a single location, I'm tempted to move it
to ipa_manager.cpp. If we want to create ELF-related helpers, then
moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
file could be useful, but would be overkill for now.

How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
The latter is more compliant with our coding style. The reason why most
other functions in this file use snake case instead of camel case is
because they mimic the glibc or libstdc++ APIs.

> +{
> +	const ElfW(Dyn) *dyn = _DYNAMIC;
> +	const ElfW(Dyn) *runpath = NULL;

%s/NULL/nullptr/

> +	const char *strtab = NULL;
> +
> +	for (; dyn->d_tag != DT_NULL; ++dyn) {

Maybe

	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {

?

Do I understand correctly that _DYNAMIC is the dynamic tags array for
the current ELF binary, and thus always corresponds to libcamera.so, not
the code calling get_runpath() ? That's what we need, but I think it
should be documented.

> +		if (dyn->d_tag == DT_STRTAB)
> +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> +		else if (dyn->d_tag == DT_RUNPATH)
> +			runpath = dyn;
> +	}
> +
> +	if (strtab && runpath)
> +		return strtab + runpath->d_un.d_val;
> +
> +	return NULL;

nullptr here too.

> +}

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

> +
>  /**
>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
>   *				     InputIt2 first2, InputIt2 last2)
Laurent Pinchart Feb. 6, 2020, 9:03 p.m. UTC | #2
Hi Kieran,

Another comment.

On Thu, Feb 06, 2020 at 08:07:38PM +0200, Laurent Pinchart wrote:
> On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
> > Provide a helper that will identify the DT_RUNPATH string from the
> > ELF dynamic library string table entries.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/include/utils.h |  1 +
> >  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> > index e467eb21c518..069b327bb436 100644
> > --- a/src/libcamera/include/utils.h
> > +++ b/src/libcamera/include/utils.h
> > @@ -33,6 +33,7 @@ namespace utils {
> >  const char *basename(const char *path);
> >  
> >  char *secure_getenv(const char *name);
> > +const char *get_runpath();
> >  
> >  template<class InputIt1, class InputIt2>
> >  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > index 4beffdab5eb6..e48cf2b6a48e 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -7,7 +7,9 @@
> >  
> >  #include "utils.h"
> >  
> > +#include <elf.h>
> >  #include <iomanip>
> > +#include <link.h>
> >  #include <sstream>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
> >  #endif
> >  }
> >  
> > +/**
> > + * \brief Obtain the runpath string from the dynamic symbol table
> 
> It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
> (.dynsym).
> 
> > + * \returns a const char pointer to the runpath entry in the elf string table
> 
> s/returns/return/
> 
> Actually after checking the doxygen documentation \returns is an alias
> for \return. Still, I think we should standardize on \return.
> 
> s/a const/A const/
> 
> Maybe s/elf/ELF/ ?
> 
> > + * or NULL if it is not found.
> 
> s/\.$//
> 
> > + */
> > +const char *get_runpath()
> 
> We only use this function in a single location, I'm tempted to move it
> to ipa_manager.cpp. If we want to create ELF-related helpers, then
> moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
> file could be useful, but would be overkill for now.
> 
> How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
> The latter is more compliant with our coding style. The reason why most
> other functions in this file use snake case instead of camel case is
> because they mimic the glibc or libstdc++ APIs.
> 
> > +{
> > +	const ElfW(Dyn) *dyn = _DYNAMIC;

bionic and musl don't seem to provide _DYNAMIC :-S They both have elf.h
and link.h, with a definition of the macros and structures used below,
but _DYNAMIC seems to be missing. I may be missing something, it should
be tested.

> > +	const ElfW(Dyn) *runpath = NULL;
> 
> %s/NULL/nullptr/
> 
> > +	const char *strtab = NULL;
> > +
> > +	for (; dyn->d_tag != DT_NULL; ++dyn) {
> 
> Maybe
> 
> 	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> 
> ?
> 
> Do I understand correctly that _DYNAMIC is the dynamic tags array for
> the current ELF binary, and thus always corresponds to libcamera.so, not
> the code calling get_runpath() ? That's what we need, but I think it
> should be documented.
> 
> > +		if (dyn->d_tag == DT_STRTAB)
> > +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> > +		else if (dyn->d_tag == DT_RUNPATH)
> > +			runpath = dyn;
> > +	}
> > +
> > +	if (strtab && runpath)
> > +		return strtab + runpath->d_un.d_val;
> > +
> > +	return NULL;
> 
> nullptr here too.
> 
> > +}
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  /**
> >   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
> >   *				     InputIt2 first2, InputIt2 last2)
Kieran Bingham Feb. 11, 2020, 2:05 p.m. UTC | #3
Hi Laurent,

On 06/02/2020 18:07, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
>> Provide a helper that will identify the DT_RUNPATH string from the
>> ELF dynamic library string table entries.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/utils.h |  1 +
>>  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index e467eb21c518..069b327bb436 100644
>> --- a/src/libcamera/include/utils.h
>> +++ b/src/libcamera/include/utils.h
>> @@ -33,6 +33,7 @@ namespace utils {
>>  const char *basename(const char *path);
>>  
>>  char *secure_getenv(const char *name);
>> +const char *get_runpath();
>>  
>>  template<class InputIt1, class InputIt2>
>>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> index 4beffdab5eb6..e48cf2b6a48e 100644
>> --- a/src/libcamera/utils.cpp
>> +++ b/src/libcamera/utils.cpp
>> @@ -7,7 +7,9 @@
>>  
>>  #include "utils.h"
>>  
>> +#include <elf.h>
>>  #include <iomanip>
>> +#include <link.h>
>>  #include <sstream>
>>  #include <stdlib.h>
>>  #include <string.h>
>> @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
>>  #endif
>>  }
>>  
>> +/**
>> + * \brief Obtain the runpath string from the dynamic symbol table
> 
> It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
> (.dynsym).
> 
>> + * \returns a const char pointer to the runpath entry in the elf string table
> 
> s/returns/return/
> 
> Actually after checking the doxygen documentation \returns is an alias
> for \return. Still, I think we should standardize on \return.

Looks like I copied \returns from the secure_getenv above.

I'll send a patch to fix that, and the other one in the source code.

 <sent>

> 
> s/a const/A const/
> 
> Maybe s/elf/ELF/ ?
> 
>> + * or NULL if it is not found.

s/NULL/nullptr here of course too

Perhaps NULL => nullptr should be in checkstyle.py.

> 
> s/\.$//
> 
>> + */
>> +const char *get_runpath()
> 
> We only use this function in a single location, I'm tempted to move it
> to ipa_manager.cpp. If we want to create ELF-related helpers, then

Indeed, as it's only a single use and not expected to be used elsewhere
it could be a local static.

At that point, the implementation can be squashed into the patch that
needs it too.


> moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
> file could be useful, but would be overkill for now.

If we end up with any more ELF support, I'd like to see it all collected
together indeed but for now it's fine.


> How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
> The latter is more compliant with our coding style. The reason why most
> other functions in this file use snake case instead of camel case is
> because they mimic the glibc or libstdc++ APIs.

I was mimicking the style of this file.

I'll move to ipa_manager and use elfRunPath

> 
>> +{
>> +	const ElfW(Dyn) *dyn = _DYNAMIC;
>> +	const ElfW(Dyn) *runpath = NULL;
> 
> %s/NULL/nullptr/
> 
>> +	const char *strtab = NULL;
>> +
>> +	for (; dyn->d_tag != DT_NULL; ++dyn) {
> 
> Maybe
> 
> 	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {


Sure but s/ELFW/ElfW/ ...

> ?
> 
> Do I understand correctly that _DYNAMIC is the dynamic tags array for
> the current ELF binary, and thus always corresponds to libcamera.so, not
> the code calling get_runpath() ? That's what we need, but I think it
> should be documented.
> 
>> +		if (dyn->d_tag == DT_STRTAB)
>> +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
>> +		else if (dyn->d_tag == DT_RUNPATH)
>> +			runpath = dyn;
>> +	}
>> +
>> +	if (strtab && runpath)
>> +		return strtab + runpath->d_un.d_val;
>> +
>> +	return NULL;
> 
> nullptr here too.
> 
>> +}
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +
>>  /**
>>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
>>   *				     InputIt2 first2, InputIt2 last2)
>
Laurent Pinchart Feb. 11, 2020, 3:58 p.m. UTC | #4
Hi Kieran,

On Tue, Feb 11, 2020 at 02:05:53PM +0000, Kieran Bingham wrote:
> On 06/02/2020 18:07, Laurent Pinchart wrote:
> > On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
> >> Provide a helper that will identify the DT_RUNPATH string from the
> >> ELF dynamic library string table entries.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/libcamera/include/utils.h |  1 +
> >>  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >> index e467eb21c518..069b327bb436 100644
> >> --- a/src/libcamera/include/utils.h
> >> +++ b/src/libcamera/include/utils.h
> >> @@ -33,6 +33,7 @@ namespace utils {
> >>  const char *basename(const char *path);
> >>  
> >>  char *secure_getenv(const char *name);
> >> +const char *get_runpath();
> >>  
> >>  template<class InputIt1, class InputIt2>
> >>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >> index 4beffdab5eb6..e48cf2b6a48e 100644
> >> --- a/src/libcamera/utils.cpp
> >> +++ b/src/libcamera/utils.cpp
> >> @@ -7,7 +7,9 @@
> >>  
> >>  #include "utils.h"
> >>  
> >> +#include <elf.h>
> >>  #include <iomanip>
> >> +#include <link.h>
> >>  #include <sstream>
> >>  #include <stdlib.h>
> >>  #include <string.h>
> >> @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
> >>  #endif
> >>  }
> >>  
> >> +/**
> >> + * \brief Obtain the runpath string from the dynamic symbol table
> > 
> > It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
> > (.dynsym).
> > 
> >> + * \returns a const char pointer to the runpath entry in the elf string table
> > 
> > s/returns/return/
> > 
> > Actually after checking the doxygen documentation \returns is an alias
> > for \return. Still, I think we should standardize on \return.
> 
> Looks like I copied \returns from the secure_getenv above.
> 
> I'll send a patch to fix that, and the other one in the source code.
> 
>  <sent>
> 
> > s/a const/A const/
> > 
> > Maybe s/elf/ELF/ ?
> > 
> >> + * or NULL if it is not found.
> 
> s/NULL/nullptr here of course too
> 
> Perhaps NULL => nullptr should be in checkstyle.py.

That's a good idea. It should be easy to add a regex that will match on
NULL, but ideally we should have a better parser that would handle the
context. "NULL" may be acceptable, as well as NULL in a comment for
instance.

Does clang-format support python plugins ? :-)

> > s/\.$//
> > 
> >> + */
> >> +const char *get_runpath()
> > 
> > We only use this function in a single location, I'm tempted to move it
> > to ipa_manager.cpp. If we want to create ELF-related helpers, then
> 
> Indeed, as it's only a single use and not expected to be used elsewhere
> it could be a local static.
> 
> At that point, the implementation can be squashed into the patch that
> needs it too.
> 
> > moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
> > file could be useful, but would be overkill for now.
> 
> If we end up with any more ELF support, I'd like to see it all collected
> together indeed but for now it's fine.
> 
> > How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
> > The latter is more compliant with our coding style. The reason why most
> > other functions in this file use snake case instead of camel case is
> > because they mimic the glibc or libstdc++ APIs.
> 
> I was mimicking the style of this file.
> 
> I'll move to ipa_manager and use elfRunPath
> 
> >> +{
> >> +	const ElfW(Dyn) *dyn = _DYNAMIC;
> >> +	const ElfW(Dyn) *runpath = NULL;
> > 
> > %s/NULL/nullptr/
> > 
> >> +	const char *strtab = NULL;
> >> +
> >> +	for (; dyn->d_tag != DT_NULL; ++dyn) {
> > 
> > Maybe
> > 
> > 	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> 
> Sure but s/ELFW/ElfW/ ...

Absolutely :-)

> > ?
> > 
> > Do I understand correctly that _DYNAMIC is the dynamic tags array for
> > the current ELF binary, and thus always corresponds to libcamera.so, not
> > the code calling get_runpath() ? That's what we need, but I think it
> > should be documented.
> > 
> >> +		if (dyn->d_tag == DT_STRTAB)
> >> +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> >> +		else if (dyn->d_tag == DT_RUNPATH)
> >> +			runpath = dyn;
> >> +	}
> >> +
> >> +	if (strtab && runpath)
> >> +		return strtab + runpath->d_un.d_val;
> >> +
> >> +	return NULL;
> > 
> > nullptr here too.
> > 
> >> +}
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> +
> >>  /**
> >>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
> >>   *				     InputIt2 first2, InputIt2 last2)
Nicolas Dufresne Feb. 12, 2020, 5:44 p.m. UTC | #5
Le mardi 11 février 2020 à 17:58 +0200, Laurent Pinchart a écrit :
> Hi Kieran,
> 
> On Tue, Feb 11, 2020 at 02:05:53PM +0000, Kieran Bingham wrote:
> > On 06/02/2020 18:07, Laurent Pinchart wrote:
> > > On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
> > > > Provide a helper that will identify the DT_RUNPATH string from the
> > > > ELF dynamic library string table entries.
> > > > 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/include/utils.h |  1 +
> > > >  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
> > > >  2 files changed, 27 insertions(+)
> > > > 
> > > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> > > > index e467eb21c518..069b327bb436 100644
> > > > --- a/src/libcamera/include/utils.h
> > > > +++ b/src/libcamera/include/utils.h
> > > > @@ -33,6 +33,7 @@ namespace utils {
> > > >  const char *basename(const char *path);
> > > >  
> > > >  char *secure_getenv(const char *name);
> > > > +const char *get_runpath();
> > > >  
> > > >  template<class InputIt1, class InputIt2>
> > > >  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> > > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > > index 4beffdab5eb6..e48cf2b6a48e 100644
> > > > --- a/src/libcamera/utils.cpp
> > > > +++ b/src/libcamera/utils.cpp
> > > > @@ -7,7 +7,9 @@
> > > >  
> > > >  #include "utils.h"
> > > >  
> > > > +#include <elf.h>
> > > >  #include <iomanip>
> > > > +#include <link.h>
> > > >  #include <sstream>
> > > >  #include <stdlib.h>
> > > >  #include <string.h>
> > > > @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
> > > >  #endif
> > > >  }
> > > >  
> > > > +/**
> > > > + * \brief Obtain the runpath string from the dynamic symbol table
> > > 
> > > It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
> > > (.dynsym).
> > > 
> > > > + * \returns a const char pointer to the runpath entry in the elf string table
> > > 
> > > s/returns/return/
> > > 
> > > Actually after checking the doxygen documentation \returns is an alias
> > > for \return. Still, I think we should standardize on \return.
> > 
> > Looks like I copied \returns from the secure_getenv above.
> > 
> > I'll send a patch to fix that, and the other one in the source code.
> > 
> >  <sent>
> > 
> > > s/a const/A const/
> > > 
> > > Maybe s/elf/ELF/ ?
> > > 
> > > > + * or NULL if it is not found.
> > 
> > s/NULL/nullptr here of course too
> > 
> > Perhaps NULL => nullptr should be in checkstyle.py.
> 
> That's a good idea. It should be easy to add a regex that will match on
> NULL, but ideally we should have a better parser that would handle the
> context. "NULL" may be acceptable, as well as NULL in a comment for
> instance.
> 
> Does clang-format support python plugins ? :-)

That is good idea since it will be very common from C and specially
GStreamer contributors. Basically any code that is copied from
somewhere else will have NULL in it.

> 
> > > s/\.$//
> > > 
> > > > + */
> > > > +const char *get_runpath()
> > > 
> > > We only use this function in a single location, I'm tempted to move it
> > > to ipa_manager.cpp. If we want to create ELF-related helpers, then
> > 
> > Indeed, as it's only a single use and not expected to be used elsewhere
> > it could be a local static.
> > 
> > At that point, the implementation can be squashed into the patch that
> > needs it too.
> > 
> > > moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
> > > file could be useful, but would be overkill for now.
> > 
> > If we end up with any more ELF support, I'd like to see it all collected
> > together indeed but for now it's fine.
> > 
> > > How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
> > > The latter is more compliant with our coding style. The reason why most
> > > other functions in this file use snake case instead of camel case is
> > > because they mimic the glibc or libstdc++ APIs.
> > 
> > I was mimicking the style of this file.
> > 
> > I'll move to ipa_manager and use elfRunPath
> > 
> > > > +{
> > > > +	const ElfW(Dyn) *dyn = _DYNAMIC;
> > > > +	const ElfW(Dyn) *runpath = NULL;
> > > 
> > > %s/NULL/nullptr/
> > > 
> > > > +	const char *strtab = NULL;
> > > > +
> > > > +	for (; dyn->d_tag != DT_NULL; ++dyn) {
> > > 
> > > Maybe
> > > 
> > > 	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> > 
> > Sure but s/ELFW/ElfW/ ...
> 
> Absolutely :-)
> 
> > > ?
> > > 
> > > Do I understand correctly that _DYNAMIC is the dynamic tags array for
> > > the current ELF binary, and thus always corresponds to libcamera.so, not
> > > the code calling get_runpath() ? That's what we need, but I think it
> > > should be documented.
> > > 
> > > > +		if (dyn->d_tag == DT_STRTAB)
> > > > +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> > > > +		else if (dyn->d_tag == DT_RUNPATH)
> > > > +			runpath = dyn;
> > > > +	}
> > > > +
> > > > +	if (strtab && runpath)
> > > > +		return strtab + runpath->d_un.d_val;
> > > > +
> > > > +	return NULL;
> > > 
> > > nullptr here too.
> > > 
> > > > +}
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > +
> > > >  /**
> > > >   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
> > > >   *				     InputIt2 first2, InputIt2 last2)
Kieran Bingham Feb. 18, 2020, 5:18 p.m. UTC | #6
Hi Laurent,

On 06/02/2020 21:03, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Another comment.
> 

>>> +{
>>> +	const ElfW(Dyn) *dyn = _DYNAMIC;
> 
> bionic and musl don't seem to provide _DYNAMIC :-S They both have elf.h
> and link.h, with a definition of the macros and structures used below,
> but _DYNAMIC seems to be missing. I may be missing something, it should
> be tested.

MUSL proved to be a real problem on this topic, so I've started a rework
to incorporate the path to a dedicated section and we'll have to
manually strip that as part of the install phase.

--
Kieran


> 42				strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> (gdb) n
> 38		for (const ElfW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> (gdb) p strtab
> $2 = 0x1560 <error: Cannot access memory at address 0x1560>
> (gdb) p dyn
> $3 = (const Elf64_Dyn *) 0x55a12b3bccc8
> (gdb) p *dyn
> $4 = {d_tag = 5, d_un = {d_val = 5472, d_ptr = 5472}}

Patch

diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
index e467eb21c518..069b327bb436 100644
--- a/src/libcamera/include/utils.h
+++ b/src/libcamera/include/utils.h
@@ -33,6 +33,7 @@  namespace utils {
 const char *basename(const char *path);
 
 char *secure_getenv(const char *name);
+const char *get_runpath();
 
 template<class InputIt1, class InputIt2>
 unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
index 4beffdab5eb6..e48cf2b6a48e 100644
--- a/src/libcamera/utils.cpp
+++ b/src/libcamera/utils.cpp
@@ -7,7 +7,9 @@ 
 
 #include "utils.h"
 
+#include <elf.h>
 #include <iomanip>
+#include <link.h>
 #include <sstream>
 #include <stdlib.h>
 #include <string.h>
@@ -70,6 +72,30 @@  char *secure_getenv(const char *name)
 #endif
 }
 
+/**
+ * \brief Obtain the runpath string from the dynamic symbol table
+ * \returns a const char pointer to the runpath entry in the elf string table
+ * or NULL if it is not found.
+ */
+const char *get_runpath()
+{
+	const ElfW(Dyn) *dyn = _DYNAMIC;
+	const ElfW(Dyn) *runpath = NULL;
+	const char *strtab = NULL;
+
+	for (; dyn->d_tag != DT_NULL; ++dyn) {
+		if (dyn->d_tag == DT_STRTAB)
+			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
+		else if (dyn->d_tag == DT_RUNPATH)
+			runpath = dyn;
+	}
+
+	if (strtab && runpath)
+		return strtab + runpath->d_un.d_val;
+
+	return NULL;
+}
+
 /**
  * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
  *				     InputIt2 first2, InputIt2 last2)