1516803 - sandbox needs to be built with --param lto-partitions=1 when GCC LTO is enabled

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Jan Hubicka]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

Built Firefox with GCC and --enable-lto


Actual results:

I got undefined symbol because LTO partitioning separated toplevel ASM statement from its use.
security/sandbox/linux/moz.build already has code to pass --param lto-partitions=1 but it seems broken. Martin Liska provided me the following patch:
diff --git a/security/sandbox/linux/moz.build b/security/sandbox/linux/moz.build
--- a/security/sandbox/linux/moz.build
+++ b/security/sandbox/linux/moz.build
@@ -99,9 +99,8 @@ if CONFIG['CC_TYPE'] in ('clang', 'gcc')
 # gcc lto likes to put the top level asm in syscall.cc in a different partition
 # from the function using it which breaks the build.  Work around that by
 # forcing there to be only one partition.
-for f in CONFIG['OS_CXXFLAGS']:
-    if f.startswith('-flto') and CONFIG['CC_TYPE'] != 'clang':
-        LDFLAGS += ['--param lto-partitions=1']
+if CONFIG['CC_TYPE'] != 'clang':
+    LDFLAGS += ['--param', 'lto-partitions=1']

 DEFINES['NS_NO_XPCOM'] = True
 DisableStlWrapping()

this solves the problem

Comment 1

[(no longer active)]

Thanks a lot for the report, Jan.  Were you planning to submit the patch through Phabricator?

Comment 2

[Jan Hubicka]

Attachment: Fix command line options for sandbox build (Bug 1516803)

Comment 3

[Thaodan]

Attachment: mozilla-1516803.patch

Updated patch that includes the requested changes from phabricator.

Comment 4

[Dmitry Butskoy (:buc)]

Thaodan,

The variant with checking OS_CXXFLAGS and f.startswith is more robast:

for f in CONFIG['OS_CXXFLAGS']:
    if f.startswith('-flto') and CONFIG['CC_TYPE'] != 'clang':
        LDFLAGS += ['--param', 'lto-partitions=1']

Linux distro can use its "own way" for compiler options, thus "-flto" can appear even when MOZ_LTO is not set...

Comment 5

[Dmitry Butskoy (:buc)]

Martin,

Since Fedora uses this patch (because of gcc+lto), could you please promote it a bit here?
(The patch is needed for SeaMonkey as well, when it will reach lto stage too).

Comment 6

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dmitry Butskoy from comment #5)

Martin,

Since Fedora uses this patch (because of gcc+lto), could you please promote it a bit here?
(The patch is needed for SeaMonkey as well, when it will reach lto stage too).

The patches here are incomplete/obsoleted. see Bug 1601903 for details how gcc profile data are different than a clang ones.
Please take the PGO patches from Firefox package we ship for Fedora:
https://src.fedoraproject.org/rpms/firefox/blob/master/f/pgo.patch