1423802 - Link commands using the C compiler are linking C++ libraries

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Mike Hommey [:glandium]]

Specifically, they are linking stdc++compat and STLPORT_LIBS through the Binary() template in build/templates.mozbuild.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1423802 - Handle stdc++compat and STLPORT_LIBS at the emitter level.

Bug 1256642 introduced magic at the emitter level to determine whether a
binary contains C++ sources and should be linked with the C compiler or
the C++ compiler.

Unfortunately, the Binary() moz.build template always adds C++ OS
libraries on Android (through STLPORT_LIBS), and C++ libraries on Linux
(stdc++compat).

The latter only ends up forcing every Binary() to be linked with the C++
linker, which is unfortunate, but doesn't cause much problems. The
former, however, involving OS libraries, the magic from bug 1256642
doesn't kick in, so we end up trying to link C++ OS libraries with the C
linker. Which ends up failing, because the libraries in STLPORT_LIBS
require -lm, which, while it's added by the C++ compiler when linking,
is not when the linkage is driven by the C compiler.

Because the fallible library, linked to all GeckoBinary()s is a C++
library, we still ended up linking with the C++ compiler on Android, so
this wasn't actually causing any problem... until I tried to remove that
fallible library in bug 1423803.

Anyways, the core problem is that moz.build evaluation is happening too
early to know whether any C++ sources are being linked together, so
there is no way the Binary() template can do the right thing. So this
change moves the logic to the emitter.

This also changes the type of STLPORT_LIBS to a list.

Review commit: https://reviewboard.mozilla.org/r/206122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206122/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1423802 - Remove the dummy fallible library.

Bug 1423803 was attempting to remove the fallible library but didn't do
so on Android because of this bug. We can now fully retire it.

Review commit: https://reviewboard.mozilla.org/r/206124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206124/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8935239 [details]
Bug 1423802 - Handle stdc++compat and STLPORT_LIBS at the emitter level.

https://reviewboard.mozilla.org/r/206122/#review211914

I'm not expert in this area, but I spent some time trying to understand what is happening here.  It looks like you're duplicating a lot of the existing functionality of the helper methods like `_link_libraries`, and I can't understand why.  That is, in a method that has a general "do these things with these variables", you're adding code that does special parts of the above rather than updating the relevant variables and then doing the general method.

Can you explain why that is so?

You might prefer to get review from somebody else who might not have the same concern.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8935240 [details]
Bug 1423802 - Remove the dummy fallible library.

https://reviewboard.mozilla.org/r/206124/#review211916

If the previous commit is good, then this will be good too.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #3)
> Comment on attachment 8935239 [details]
> Bug 1423802 - Handle stdc++compat and STLPORT_LIBS at the emitter level.
> 
> https://reviewboard.mozilla.org/r/206122/#review211914
> 
> I'm not expert in this area, but I spent some time trying to understand what
> is happening here.  It looks like you're duplicating a lot of the existing
> functionality of the helper methods like `_link_libraries`, and I can't
> understand why.  That is, in a method that has a general "do these things
> with these variables", you're adding code that does special parts of the
> above rather than updating the relevant variables and then doing the general
> method.
> 
> Can you explain why that is so?

Because the relevant variables can only be updated after obj.cxx_link is final, which it isn't before every other library listed in the relevant variable has already been handled.

I updated the patch to make things clearer in the final code, at the expense of more churn in the patch itself.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8935239 [details]
Bug 1423802 - Handle stdc++compat and STLPORT_LIBS at the emitter level.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206122/diff/1-2/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8935240 [details]
Bug 1423802 - Remove the dummy fallible library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206124/diff/1-2/

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8935239 [details]
Bug 1423802 - Handle stdc++compat and STLPORT_LIBS at the emitter level.

https://reviewboard.mozilla.org/r/206122/#review212620

Thanks for reworking this; it's much more clear.  And the commit message was first rate in the first version, too!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7fcb5272323
Handle stdc++compat and STLPORT_LIBS at the emitter level. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/8c677030915b
Remove the dummy fallible library. r=nalexander

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d7fcb5272323
https://hg.mozilla.org/mozilla-central/rev/8c677030915b