Closed Bug 749518 Opened 9 years ago Closed 9 years ago

Use dl_iterate_phdr instead of proc/maps for Linux symbolicate

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(3 files, 3 obsolete files)

We need to use dl_iterate_phdr for get the real segments to support the elfhack configuration.
Component: Graphics → Gecko Profiler
QA Contact: thebes → gecko-profiler
Depends on: 783331
Attached patch patch (obsolete) — Splinter Review
With this I can profile with elfhack / no elfhack.

Right now I'm turning it on for Android first because I want to test other platforms before changing the code.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #656239 - Flags: review?(mh+mozilla)
Comment on attachment 656239 [details] [diff] [review]
patch

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

::: mozglue/android/APKOpen.cpp
@@ +45,5 @@
>  #define RUSAGE_THREAD 1
>  #endif
>  
> +extern "C" NS_EXPORT
> +int dl_iterate_phdr(

Please remove this. This was only to get you something working.

::: tools/profiler/shared-libraries-linux.cc
@@ +28,5 @@
>   return strlen(*lineptr);
>  }
>  #endif
>  
> +#ifdef ANDROID

You need to check for MOZ_OLD_LINKER not being defined.

@@ +47,5 @@
> +                                     for this object */
> +    Elf32_Half       dlpi_phnum; /* # of items in dlpi_phdr */
> +};
> +extern "C"
> +int dl_iterate_phdr(

I'm surprised you don't need a macro that expends to __attribute__((visibility("default"))) here. Please make it __attribute__((weak)), so that we don't need the dummy declaration in APKOpen.cpp.

@@ +80,5 @@
> +    if (start < libStart)
> +      libStart = start;
> +    if (end > libEnd)
> +      libEnd = end;
> +  }

You can simplify this by starting with libStart = -1, libEnd = 0, and going through all PT_LOADs in a single loop. (That's what our linker does, fwiw)
Attachment #656239 - Flags: review?(mh+mozilla) → review-
Attached patch patch v2 (obsolete) — Splinter Review
My previous patch had the dl_phdr_info definition hard coded to the 32-bit definition. I now use the definition from ElfLoader.h. If you mind me exporting these headers I could add a single header to wrap the functionality I need from dl_iterate_phdr inside mozglue and only export a single function.
Attachment #656239 - Attachment is obsolete: true
Attachment #656458 - Flags: review?(mh+mozilla)
Comment on attachment 656458 [details] [diff] [review]
patch v2

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

::: mozglue/linker/Makefile.in
@@ +38,5 @@
> +  ElfLoader.h \
> +  Elfxx.h \
> +  Utils.h \
> +  Zip.h \
> +  $(NULL)

I'm fine with you using the ElfLoader.h header, but not exporting them. Please just add the right INCLUDES instead.

::: tools/profiler/shared-libraries-linux.cc
@@ +28,5 @@
>   return strlen(*lineptr);
>  }
>  #endif
>  
> +#ifndef MOZ_OLD_LINKER

This isn't defined by default, so you need to add it to Makefile.in.

@@ +31,5 @@
>  
> +#ifndef MOZ_OLD_LINKER
> +// TODO fix me with proper include
> +#include "nsDebug.h"
> +#include "ElfLoader.h" // dl_phdr_info

You should make that conditional to ANDROID, and use the standard link.h otherwise.

@@ +117,2 @@
>      SharedLibrary shlib(start, end, offset, name);
>      info.AddSharedLibrary(shlib);

How does SharedLibrary handle the fact that things from /proc/pid/maps already is there from dl_iterate_phdr output? Shouldn't you just skip /proc/pid/maps when using dl_iterate_phdr on non-Android? (although, maybe you left it for JS JIT segments ?)
Attachment #656458 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 656458 [details] [diff] [review]
patch v2

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

::: tools/profiler/shared-libraries-linux.cc
@@ +77,5 @@
> +
> +#ifndef MOZ_OLD_LINKER
> +  dl_iterate_phdr(dl_iterate_callback, &info);
> +#ifndef ANDROID
> +  return info;

RE your last comment, I return early if it's not android. And when it's android I only accept '/dev/ashmem/dalvik-jit-code-cache'.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #656458 - Attachment is obsolete: true
Attachment #656496 - Flags: review?(mh+mozilla)
Comment on attachment 656496 [details] [diff] [review]
patch v3

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

::: tools/profiler/Makefile.in
@@ +73,5 @@
> +endif
> +
> +ifeq ($(OS_TARGET),Android)
> +# The use of dl_iterate_phdr depends on mozglue custom android linker
> +LOCAL_INCLUDES = \

+= (there are other LOCAL_INCLUDES earlier in the file)

@@ +74,5 @@
> +
> +ifeq ($(OS_TARGET),Android)
> +# The use of dl_iterate_phdr depends on mozglue custom android linker
> +LOCAL_INCLUDES = \
> +  -I$(srcdir)/../../mozglue/linker \

$(topsrcdir)/mozglue/linker

::: tools/profiler/shared-libraries-linux.cc
@@ +40,5 @@
> +#include <features.h>
> +#include <dlfcn.h>
> +#include <sys/types.h>
> +
> +__attribute__((weak))

Please add a #ifdef ANDROID around the attribute
Attachment #656496 - Flags: review?(mh+mozilla) → review+
going to land once https://hg.mozilla.org/try/rev/5792af7c00f2 is green.
Attached patch patch v4Splinter Review
Attachment #656496 - Attachment is obsolete: true
Attachment #656892 - Flags: review+
(In reply to Benoit Girard (:BenWa) from comment #8)
> going to land once https://hg.mozilla.org/try/rev/5792af7c00f2 is green.

Correct URL: https://tbpl.mozilla.org/?tree=Try&rev=5792af7c00f2
https://hg.mozilla.org/mozilla-central/rev/a8b73286ba78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
I didn't test the latest version of the patch and I realize now that it's causing a crash because dl_iterate_phdr is null which means the symbol isn't being resolve.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This broke B2G gb_armv7a_gecko builds.

Backed out:
https://hg.mozilla.org/mozilla-central/rev/938dacff2d5c
Target Milestone: mozilla18 → ---
Ok, just seen bug 771653 comment 15, so seems like we don't care about GB builds any more (this just wasn't communicated to the sheriffs), so have relanded this. Sorry for the noise.

Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96a49384202
Whiteboard: [leave open]
(In reply to Benoit Girard (:BenWa) from comment #13)
> I didn't test the latest version of the patch and I realize now that it's
> causing a crash because dl_iterate_phdr is null which means the symbol isn't
> being resolve.

extern "C" is missing for the symbol not to be C++-mangled. That's an oversight on my part.
Attached patch Add extern "C"Splinter Review
Attachment #657884 - Flags: review?(mh+mozilla)
Comment on attachment 657884 [details] [diff] [review]
Add extern "C"

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

Another problem I just spotted is that B2G doesn't have dl_iterate_phdr (and it defines ANDROID). :(
Attachment #657884 - Flags: review?(mh+mozilla) → review+
This does the trick for me.
Attachment #658027 - Flags: review?(bgirard)
Assignee: bgirard → mh+mozilla
Assignee: mh+mozilla → bgirard
Attachment #658027 - Flags: review?(bgirard) → review+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/55e089793de8
https://hg.mozilla.org/mozilla-central/rev/67f07d04c5ad
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 788974
You need to log in before you can comment on or make changes to this bug.