1077384 - Make libmozglue a pseudo-LD_PRELOAD on android

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Make libmozglue a pseudo-LD_PRELOAD on android

In order to avoid adding more dlsym overhead than there already is, resolve
symbols directly in the library containing the linker. (GetSymbolPtr is
essentially free ; dlsym makes the system linker compule a ElfHash itself,
and that's quite expensive to do on all symbols)

This also paves the way for direct symbol resolution in all system libraries.

In terms of code layout, I want BaseElf to merge with LibHandle eventually,
once SystemElf switches over, which explains why I moved code around in a
separate file.

Comment 2

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efbd75e19459

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efbd75e19459

So, this is interesting... this is all orange, but the build works on my phone...

Comment 4

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efbd75e19459
> 
> So, this is interesting... this is all orange, but the build works on my
> phone...

This may well have been my ANDROID_PACKAGE_NAME trick that it didn't like. Trying again without:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=50d8b90140ce

Comment 5

[Mike Hommey [:glandium]]

Attachment: Make libmozglue a pseudo-LD_PRELOAD on android

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52ad66234009
Turns out all the orange was... lack of reentrancy in dl* functions on Android < 4.1.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Make libmozglue a pseudo-LD_PRELOAD on android

Huh, with a return type for Init, it should be better
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8bbb4a29d456

Comment 7

[Mike Hommey [:glandium]]

Attachment: Make libmozglue a pseudo-LD_PRELOAD on android

Sigh, still missing return type, in the implementation this time. I *will* get this right dammit.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1929ef22cf2d

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8499946 [details] [diff] [review]
Make libmozglue a pseudo-LD_PRELOAD on android

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

::: mozglue/linker/CustomElf.cpp
@@ +362,5 @@
>    for (std::vector<RefPtr<LibHandle> >::const_iterator it = dependencies.begin();
>         it < dependencies.end(); ++it) {
>      if (!(*it)->IsSystemElf()) {
> +      sym = static_cast<BaseElf *>(
> +        reinterpret_cast<CustomElf *>((*it).get()))->GetSymbolPtr(symbol, hash);

The static_cast here isn't necessary, is it?  I don't see a GetSymbolPtr override in CustomElf.  Also, I think this reinterpret_cast can be static_cast.

Also, more of a code cleanliness idea than anything: this IsSystemElf/reinterpret_cast stuff seems to happen fairly often, you could just consider:

  if (CustomElf* ce = (*it)->AsCustomElf()) {
    ...
  }

or similar.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> Comment on attachment 8499946 [details] [diff] [review]
> Make libmozglue a pseudo-LD_PRELOAD on android
> 
> Review of attachment 8499946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/linker/CustomElf.cpp
> @@ +362,5 @@
> >    for (std::vector<RefPtr<LibHandle> >::const_iterator it = dependencies.begin();
> >         it < dependencies.end(); ++it) {
> >      if (!(*it)->IsSystemElf()) {
> > +      sym = static_cast<BaseElf *>(
> > +        reinterpret_cast<CustomElf *>((*it).get()))->GetSymbolPtr(symbol, hash);
> 
> The static_cast here isn't necessary, is it?  I don't see a GetSymbolPtr
> override in CustomElf.

There is CustomElf::GetSymbolPtr(const char *symbol), and that makes the static_cast necessary.

> Also, I think this reinterpret_cast can be
> static_cast.
> 
> Also, more of a code cleanliness idea than anything: this
> IsSystemElf/reinterpret_cast stuff seems to happen fairly often, you could
> just consider:
> 
>   if (CustomElf* ce = (*it)->AsCustomElf()) {
>     ...
>   }
> 
> or similar.

Will file a followup.

Comment 10

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1604224596

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c1604224596