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.

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?

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

(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