Last Comment Bug 747033 - Implement dl_iterate_phdr in the android linker
: Implement dl_iterate_phdr in the android linker
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: mozglue (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
:
: Mike Hommey [:glandium]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-19 09:06 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-04-25 07:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement dl_iterate_phdr in the custom linker (17.43 KB, patch)
2012-04-23 03:41 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Implement dl_iterate_phdr in the custom linker (16.88 KB, patch)
2012-04-23 08:36 PDT, Mike Hommey [:glandium]
nfroyd: review+
Details | Diff | Splinter Review

Description User image Jeff Muizelaar [:jrmuizel] 2012-04-19 09:06:05 PDT
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 User image Mike Hommey [:glandium] 2012-04-19 09:30:46 PDT
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 User image Benoit Girard (:BenWa) 2012-04-19 10:12:36 PDT
We're running into this problem in both Android and Linux. Is this a problem that's caused by our custom loader?
Comment 3 User image Mike Hommey [:glandium] 2012-04-19 23:15:00 PDT
(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 User image Benoit Girard (:BenWa) 2012-04-19 23:24:03 PDT
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 User image Mike Hommey [:glandium] 2012-04-23 03:41:59 PDT
Created attachment 617443 [details] [diff] [review]
Implement dl_iterate_phdr in the custom linker
Comment 6 User image Mike Hommey [:glandium] 2012-04-23 03:52:08 PDT
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 User image :Ehsan Akhgari 2012-04-23 08:25:59 PDT
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 User image Mike Hommey [:glandium] 2012-04-23 08:36:03 PDT
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.
Comment 9 User image :Ehsan Akhgari 2012-04-23 08:37:28 PDT
(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 User image (dormant account) 2012-04-23 12:05:14 PDT
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 User image Nathan Froyd [:froydnj] 2012-04-24 06:42:57 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.