On Wed, Feb 15, 2017 at 10:57 AM, Matt Turner <matts...@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 4:03 AM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
> > Hi Matt,
> >
> > Afaics dl_iterate_phdr is available on musl, (some?) BSDs and Solaris
> > - thank you for opting for it.
> >
> > Out of curiosity:
> > Have you checked if on those platforms the "GNU\0" strcmp is
> > applicable and not another string ? Worth adding a note/comment ?
>
> It's generated by GNU ld in all cases I'm aware of, so I think it's the
> same.
>
> > On 14 February 2017 at 23:58, Matt Turner <matts...@gmail.com> wrote:
> >> Provides the ability to read the .note.gnu.build-id section of ELF
> >> binaries, which is inserted by the --build-id=... flag to ld.
> >> ---
> >> configure.ac | 2 +
> >> src/util/Makefile.sources | 2 +
> >> src/util/build_id.c | 110 ++++++++++++++++++++++++++++++
> ++++++++++++++++
> >> src/util/build_id.h | 34 ++++++++++++++
> >> 4 files changed, 148 insertions(+)
> >> create mode 100644 src/util/build_id.c
> >> create mode 100644 src/util/build_id.h
> >>
> >> diff --git a/configure.ac b/configure.ac
> >> index f001743..99c74f0 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -768,6 +768,8 @@ LIBS="$LIBS $DLOPEN_LIBS"
> >> AC_CHECK_FUNCS([dladdr])
> >> LIBS="$save_LIBS"
> >>
> >> +AC_CHECK_FUNC([dl_iterate_phdr], [DEFINES="$DEFINES
> -DHAVE_DL_ITERATE_PHDR"])
> >> +
> > What we want here is a local (in-configure) variable -> have_foo,
> > which will be checked by ANV/others that depend on an actual
> > implementation.
> > As-is things will compile but we'll get a link error.
> >
> >
> >> +const struct build_id_note *
> >> +build_id_find_nhdr(const char *filename)
> >> +{
> >> + struct callback_data data = {
> >> + .filename = filename,
> >> + .note = NULL,
> >> + };
> >> +
> >> + if (dl_iterate_phdr(build_id_find_nhdr_callback, &data)) {
> >> + return data.note;
> >> + } else {
> >> + return NULL;
> >> + }
> > Nit:
> >
> > if (!dl_iterate_phdr(build_id_find_nhdr_callback, &data))
> > return NULL;
> >
> > return data.note;
> >
> >
> >> diff --git a/src/util/build_id.h b/src/util/build_id.h
> >> new file mode 100644
> >> index 0000000..20db4ac
> >> --- /dev/null
> >> +++ b/src/util/build_id.h
> >
> >> +struct build_id_note;
> >> +
> >> +const struct build_id_note *
> >> +build_id_find_nhdr(const char *filename);
> >> +
> >> +unsigned
> >> +build_id_length(const struct build_id_note *note);
> >> +
> >> +void
> >> +build_id_read(const struct build_id_note *note,
> >> + unsigned char *build_id, size_t n);
> >
> > With the configure fix, one can bring back the stubs here. Having a
> > function declaration w/o a definition is a bit iffy.
>
> I think having stubs was a bad idea from the outset. In any case one
> of these stubs is called, something bad is going to happen.
>
> I'll wrap the header in #ifdef HAVE_DL_ITERATE_PHDR instead.
>
For the sake of other code which needs to #if around whether or not to call
these functions, it might be a good idea to have a
#define HAVE_BUILD_ID
inside of the #ifdef HAVE_DL_ITERATE_PHDR. It's a bit more convenient and
allows for a potential future where we have some method for getting a
build-id on windows or similar.
--Jason
> > With the above:
> > Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>
> Thanks. I'll incorporate these changes and send a v3.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
