Closed Bug 1412405 Opened 2 years ago Closed 2 years ago

try harder to find a definition for isnanf in the custom linker

Categories

(Core :: mozglue, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

The comment with the accompanying change explains things, but the short
version is that clang generates full calls to isnanf, which our
dlsym-based symbol lookup in the custom linker cannot handle correctly.
We therefore need to do extra work for isnanf to find the correct symbol.
Nick, feel free to pass this off to glandium if you don't feel up to reviewing
this.  I've tried to explain the rationale in the commit message and the
comment; the pointed-to chromium issue also has some information.
Attachment #8922946 - Flags: review?(nalexander)
Blocks: 1163171
Comment on attachment 8922946 [details] [diff] [review]
try harder to find a definition for isnanf in the custom linker

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

Great comment and links; thanks.  I just saw comments about xpcshell tests _not_ using our custom linker; can you explain why or why not this work-around is or is not required there?
Attachment #8922946 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #2)
> Great comment and links; thanks.  I just saw comments about xpcshell tests
> _not_ using our custom linker; can you explain why or why not this
> work-around is or is not required there?

Indeed, the xpcshell tests work fine without this hack.

My understanding of this is as follows: For Fennec, we're not loading libxul via the normal means; we're going to load it out of the APK via the custom linker.  Note that in the APK it's not stored as a ELF file; it's been compressed and suchlike so the APK is smaller.  So here the custom linker is responsible for loading and its symbol resolution scheme is to fall back on `dlsym`, which as the patch explains is busted for this particular case.

xpcshell, by contrast, is an ordinary ELF executable: we're linking to the system libraries and executing xpcshell.  We're loading the uncompressed libxul.so as a normal shared library, as xpcshell depends on it, and the system linker (which understands what linking to a weak `isinf` symbol means) is responsible for symbol resolution, so everything works out OK.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Nick Alexander :nalexander from comment #2)
> > Great comment and links; thanks.  I just saw comments about xpcshell tests
> > _not_ using our custom linker; can you explain why or why not this
> > work-around is or is not required there?
> 
> Indeed, the xpcshell tests work fine without this hack.
> 
> My understanding of this is as follows: For Fennec, we're not loading libxul
> via the normal means; we're going to load it out of the APK via the custom
> linker.  Note that in the APK it's not stored as a ELF file; it's been
> compressed and suchlike so the APK is smaller.  So here the custom linker is
> responsible for loading and its symbol resolution scheme is to fall back on
> `dlsym`, which as the patch explains is busted for this particular case.
> 
> xpcshell, by contrast, is an ordinary ELF executable: we're linking to the
> system libraries and executing xpcshell.  We're loading the uncompressed
> libxul.so as a normal shared library, as xpcshell depends on it, and the
> system linker (which understands what linking to a weak `isinf` symbol
> means) is responsible for symbol resolution, so everything works out OK.

OK, that makes sense.  xpcshell is weird in this case :)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eeb19b262cc
try harder to find a definition for isnanf in the custom linker; r=nalexander
Comment on attachment 8922946 [details] [diff] [review]
try harder to find a definition for isnanf in the custom linker

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

You should probably do the same as for libc. See bug #1081034.
Attachment #8922946 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/3eeb19b262cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Mike Hommey [:glandium] from comment #6)
> You should probably do the same as for libc. See bug #1081034.

5:50 PM <froydnj> glandium: if https://hg.mozilla.org/try/rev/4c7cfb032a198610cfad0ae071060f31d40d6525#l1.54 works, are you happy with that?
5:51 PM <@glandium> froydnj: yes

Going to land that, then.  Needed an explicit template instantiation to work around some issues.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/339e44e41f22
fix isnanf symbol lookup by using a LoadedElf for libm; r=glandium
Depends on: 1414506
You need to log in before you can comment on or make changes to this bug.