749518 - Use dl_iterate_phdr instead of proc/maps for Linux symbolicate

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

We need to use dl_iterate_phdr for get the real segments to support the elfhack configuration.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

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.

Comment 2

[Mike Hommey [:glandium]]

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)

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch v2

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.

Comment 4

[Mike Hommey [:glandium]]

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 ?)

Comment 5

[Benoit Girard (:BenWa)]

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'.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch v3

Comment 7

[Mike Hommey [:glandium]]

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

Comment 8

[Benoit Girard (:BenWa)]

going to land once https://hg.mozilla.org/try/rev/5792af7c00f2 is green.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v4

Comment 10

[Benoit Girard (:BenWa)]

(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

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 656892 [details] [diff] [review]
patch v4

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8b73286ba78

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Benoit Girard (:BenWa)]

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.

Comment 14

[Ed Morley [:emorley]]

This broke B2G gb_armv7a_gecko builds.

Backed out:
https://hg.mozilla.org/mozilla-central/rev/938dacff2d5c

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Mike Hommey [:glandium]]

(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.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: Add extern "C"

Comment 19

[Mike Hommey [:glandium]]

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). :(

Comment 20

[Mike Hommey [:glandium]]

Attachment: Don't use dl_iterate_phdr on gonk

This does the trick for me.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 657884 [details] [diff] [review]
Add extern "C"

https://hg.mozilla.org/integration/mozilla-inbound/rev/67f07d04c5ad

Comment 22

[Benoit Girard (:BenWa)]

Comment on attachment 658027 [details] [diff] [review]
Don't use dl_iterate_phdr on gonk

https://hg.mozilla.org/integration/mozilla-inbound/rev/55e089793de8

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55e089793de8
https://hg.mozilla.org/mozilla-central/rev/67f07d04c5ad