747033 - Implement dl_iterate_phdr in the android linker

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

We need this so that we can discover the libraries that we have loaded for use in the profiler. Currently, we use /proc/maps and this is bad. I believe we actually only need dlpi_addr and dlpi_name for our use case.

Comment 1

[Mike Hommey [:glandium]]

You may want to keep both dlpi_addr and /proc/maps info to correlate (since dl_iterate_phdr won't give you where the regions end, except if we add phdr info, in which case you could parse the PT_LOAD entries)

Comment 2

[Benoit Girard (:BenWa)]

We're running into this problem in both Android and Linux. Is this a problem that's caused by our custom loader?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> We're running into this problem in both Android and Linux. Is this a problem
> that's caused by our custom loader?

If "this problem" is the number of segments in /proc/self/maps for libxul.so, it is due to elfhack.

Comment 4

[Benoit Girard (:BenWa)]

That explains why I'm able to profile my builds but Jeff can't profile is. I have elf hack disabled.

Looks like we should disable elfhack for now and work towards implementing dl_iterate_phr.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Implement dl_iterate_phdr in the custom linker

Comment 6

[Mike Hommey [:glandium]]

With the attached patch, you should be able to get the information you need. Please note you'll have to define the function yourself on Android, as the headers don't have a definition.

You however won't need a complete or totally accurate definition, the following should suffice:
#ifndef HAVE_DL_ITERATE_PHDR
struct dl_phdr_info {
  void *dlpi_addr;
  const char *dlpi_name;
};

int dl_iterate_phdr(int (*callback)(struct dl_phdr_info *, size_t, void *), void *data)  __attribute__((weak));
#endif

Also note that the old Android linker, still in use for XUL fennec, won't have dl_iterate_phdr, so you want to make its use conditional to MOZ_OLD_LINKER not being defined (it's not an AC_DEFINE, so you'll need the usual ifdef/DEFINES += construct for that).

You may want to r? or f? me on your patch using dl_iterate_phdr :)

Comment 7

[(no longer active)]

Comment on attachment 617443 [details] [diff] [review]
Implement dl_iterate_phdr in the custom linker

__wrap_dl_iterate_phdr should abort the loop if the callback returns a non-zero value.

Also, technically you should init dlpi_phdr and dpli_phnum in every iteration of the loop, since a broken callback function might overwrite them, and see the changed values in the following calls.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Implement dl_iterate_phdr in the custom linker

Ehsan is right, I missed the part about the callback return value in the dl_iterate_phdr manpage.

Comment 9

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #8)
> Created attachment 617488 [details] [diff] [review]
> Implement dl_iterate_phdr in the custom linker
> 
> Ehsan is right, I missed the part about the callback return value in the
> dl_iterate_phdr manpage.

I knew to look for it cause I also missed it in my own dl_iterate_phdr implementation.  :-)

Comment 10

[(dormant account)]

Comment on attachment 617488 [details] [diff] [review]
Implement dl_iterate_phdr in the custom linker

I think Nathan should displace me on linker stuff

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 617488 [details] [diff] [review]
Implement dl_iterate_phdr in the custom linker

Review of attachment 617488 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the change below.

::: mozglue/linker/ElfLoader.h
@@ +38,5 @@
> +  struct dl_phdr_info {
> +    Elf::Addr dlpi_addr;
> +    const char *dlpi_name;
> +    const Elf::Phdr *dlpi_phdr;
> +    uint16_t dlpi_phnum;

Please define Elf::Half in Elfxx.h and use it here for consistency's sake.

Comment 12

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9548c09c19

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/bb9548c09c19