Closed Bug 1377971 Opened 2 years ago Closed 2 years ago

honor LIB_IS_C_ONLY in more cases

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files)

No description provided.
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.
Attachment #8883109 - Flags: review?(cmanchester)
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.
Attachment #8883110 - Flags: review?(cmanchester)
Attachment #8883109 - Flags: review?(cmanchester) → review?(mh+mozilla)
Attachment #8883110 - Flags: review?(cmanchester) → review?(mh+mozilla)
Attachment #8883109 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8883110 - Flags: review?(mh+mozilla) → review+
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.)
Attachment #8883578 - Flags: review?(giles)
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.
Attachment #8883578 - Flags: review?(giles) → review+
Pushed by nfroyd@mozilla.com:
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
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd48ef4aea6d
followup - don't explicitly link to libm on Windows; r=bustage
(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!
Product: Core → Firefox Build System
Duplicate of this bug: 1305960
You need to log in before you can comment on or make changes to this bug.