If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make libmozglue a pseudo-LD_PRELOAD on android

RESOLVED FIXED in mozilla35

Status

()

Core
mozglue
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8499528 [details] [diff] [review]
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.
Attachment #8499528 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Blocks: 1077148
(Assignee)

Comment 2

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efbd75e19459
(Assignee)

Comment 3

3 years ago
(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...
(Assignee)

Comment 4

3 years ago
(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
(Assignee)

Updated

3 years ago
Blocks: 1077366
(Assignee)

Comment 5

3 years ago
Created attachment 8499904 [details] [diff] [review]
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.
Attachment #8499904 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8499528 - Attachment is obsolete: true
Attachment #8499528 - Flags: review?(nfroyd)
(Assignee)

Comment 6

3 years ago
Created attachment 8499942 [details] [diff] [review]
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
Attachment #8499942 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8499904 - Attachment is obsolete: true
Attachment #8499904 - Flags: review?(nfroyd)
(Assignee)

Comment 7

3 years ago
Created attachment 8499946 [details] [diff] [review]
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
Attachment #8499946 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8499942 - Attachment is obsolete: true
Attachment #8499942 - Flags: review?(nfroyd)
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.
Attachment #8499946 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1604224596
https://hg.mozilla.org/mozilla-central/rev/7c1604224596
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Updated

3 years ago
Blocks: 1081034
You need to log in before you can comment on or make changes to this bug.