1377971 - honor LIB_IS_C_ONLY in more cases

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make C++ linking for Linkable ignore shared libraries

We have a flag set on all Linkables, cxx_link, denoting whether there's
anything being linked into them that requires C++.  We do this even when
we link against shared libraries that required C++.  But if these
libraries don't export C++ interfaces, there's no reason that the things
linking against them should require C++.  Therefore, ignore shared
libraries when making the determination of whether an object requires
C++ or not.

I think we *could* have problems here if the C code referenced C++ symbols
through inline asm or similar.  But I think this is *really* unlikely.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - honor LIB_IS_C_ONLY in more cases

We currently only honor LIB_IS_C_ONLY for cases where we set a
LIBRARY (and, implicitly, REAL_LIBRARY) and FORCE_SHARED_LIB.  For many
libraries, such as the libraries from NSS, we never set REAL_LIBRARY,
which leads to not setting REAL_LIBRARY, which leads to not honoring
LIB_IS_C_ONLY.  This practice has not been harmful thus far (except
perhaps linking in more things than necessary to our NSS shared
libraries), but on some platforms, linking with the C++ compiler will
drag in more things than we would like.

Consulting LIBRARY first should not be necessary; checking
FORCE_SHARED_LIB should be enough to tell us if we're building a shared
library for the purposes of honoring LIB_IS_C_ONLY.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8883110 [details] [diff] [review]
part 2 - honor LIB_IS_C_ONLY in more cases

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

"we never set REAL_LIBRARY, which leads to not setting REAL_LIBRARY,"

One of those is probably meant to be LIBRARY.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1a - link libavutil and libavcodec with libm

Both of these libraries call into libm for various reasons, but by
linking with the C++ compiler on most platforms, they never had to
declare their dependency on libm.  Future changes will make these
libraries link with the C compiler, which won't automatically link with
libm, so we need to make the dependency explicit prior to that change.

(I have seen the other two patches on the bug fail on try without this change.
They also fail in a local x86-64 Linux build, but I have not seen the same
failure on try, which seems peculiar to me.  Nonetheless, linking to libm
everywhere seems like the correct thing to do.)

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8883578 [details] [diff] [review]
part 1a - link libavutil and libavcodec with libm

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

::: media/ffvpx/libavutil/moz.build
@@ +54,5 @@
>  NO_VISIBILITY_FLAGS = True
>  
>  OS_LIBS += CONFIG['REALTIME_LIBS']
> +OS_LIBS += ['m']
> +    

Please remove the trailing whitespace here.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f41d6e14484
part 1 - make C++ linking for Linkable ignore shared libraries; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/de188948af11
part 2 - link libavutil and libavcodec with libm; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06c107c096b
part 3 - honor LIB_IS_C_ONLY in more cases; r=glandium

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd48ef4aea6d
followup - don't explicitly link to libm on Windows; r=bustage

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> (I have seen the other two patches on the bug fail on try without this
> change.
> They also fail in a local x86-64 Linux build, but I have not seen the same
> failure on try, which seems peculiar to me.  Nonetheless, linking to libm
> everywhere seems like the correct thing to do.)

Did your local build have `--enable-stdcxx-compat`? per bug 1256642 comment 2 that was the thing that made a mess of things in my original work. Thanks for fixing this!

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f41d6e14484
https://hg.mozilla.org/mozilla-central/rev/de188948af11
https://hg.mozilla.org/mozilla-central/rev/b06c107c096b
https://hg.mozilla.org/mozilla-central/rev/fd48ef4aea6d