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

Status: RESOLVED FIXED

(Core :: mozglue, enhancement)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

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

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

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?

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Nick Alexander :nalexander [he/him]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eeb19b262cc
try harder to find a definition for isnanf in the custom linker; r=nalexander

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3eeb19b262cc

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/339e44e41f22
fix isnanf symbol lookup by using a LoadedElf for libm; r=glandium

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/339e44e41f22