1302891 - Add Clang CFI support to build system

Status: NEW

(Firefox Build System :: General, defect)

Description

[Enes Göktaş [:egoktas]]

Add -p (linux64-cfi) option that will build Firefox with CFI in Clang on Linux x64.

Comment 1

[Enes Göktaş [:egoktas]]

Instead of adding the linux64-cfi, we will add a

Comment 2

[Enes Göktaş [:egoktas]]

Instead of adding the "linux64-cfi" platform option, we will add a configuration flag "--enable-cfi" which will prepare the build configuration such that Firefox gets built with CFI in Clang.

The process of fixing this bug will include the necessary patches to the build system, such that the build system will support enabling CFI.

Comment 3

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 0 - Required user changes to enable CFI;

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

Comment 4

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

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

Comment 5

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 4 - Disable CFI for binaries that are compiled with -fno-lto;

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

Comment 6

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part x - Wrap libxul.so linker to prevent getting the linking process killed (Bug 1303227)

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

Comment 7

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part x - Skip check_vanilla_allocations.py test (Bug 1303236)

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

Comment 8

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 1 - Add --enable-cfi configuration flag;

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

Comment 9

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

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

Comment 10

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

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

Comment 11

[Enes Göktaş [:egoktas]]

Comment on attachment 8791765 [details]
Bug 1302891 - Part 0 - Required user changes to enable CFI;

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

Comment 12

[Enes Göktaş [:egoktas]]

Comment on attachment 8791766 [details]
Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

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

Comment 13

[Enes Göktaş [:egoktas]]

Comment on attachment 8791767 [details]
Bug 1302891 - Part 4 - Disable CFI for binaries that are compiled with -fno-lto;

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

Comment 14

[Enes Göktaş [:egoktas]]

Comment on attachment 8791768 [details]
Bug 1302891 - Part x - Wrap libxul.so linker to prevent getting the linking process killed (Bug 1303227)

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

Comment 15

[Enes Göktaş [:egoktas]]

Comment on attachment 8791769 [details]
Bug 1302891 - Part x - Skip check_vanilla_allocations.py test (Bug 1303236)

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

Comment 16

[Enes Göktaş [:egoktas]]

Comment on attachment 8791770 [details]
Bug 1302891 - Part 1 - Add --enable-cfi configuration flag;

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

Comment 17

[Enes Göktaş [:egoktas]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

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

Comment 18

[Enes Göktaş [:egoktas]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

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

Comment 19

[Enes Göktaş [:egoktas]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

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

Comment 20

[Enes Göktaş [:egoktas]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

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

Comment 21

[Enes Göktaş [:egoktas]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79064/diff/3-4/

Comment 22

[Enes Göktaş [:egoktas]]

Comment on attachment 8791765 [details]
Bug 1302891 - Part 0 - Required user changes to enable CFI;

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

Comment 23

[Enes Göktaş [:egoktas]]

Comment on attachment 8791766 [details]
Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

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

Comment 24

[Enes Göktaş [:egoktas]]

Comment on attachment 8791767 [details]
Bug 1302891 - Part 4 - Disable CFI for binaries that are compiled with -fno-lto;

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

Comment 25

[Enes Göktaş [:egoktas]]

Comment on attachment 8791770 [details]
Bug 1302891 - Part 1 - Add --enable-cfi configuration flag;

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

Comment 26

[Enes Göktaş [:egoktas]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79080/diff/3-4/

Comment 27

[Enes Göktaş [:egoktas]]

Comment on attachment 8791768 [details]
Bug 1302891 - Part x - Wrap libxul.so linker to prevent getting the linking process killed (Bug 1303227)

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

Comment 28

[Enes Göktaş [:egoktas]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79064/diff/4-5/

Comment 29

[Enes Göktaş [:egoktas]]

Comment on attachment 8791769 [details]
Bug 1302891 - Part x - Skip check_vanilla_allocations.py test (Bug 1303236)

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

Comment 30

[Enes Göktaş [:egoktas]]

Comment on attachment 8791769 [details]
Bug 1302891 - Part x - Skip check_vanilla_allocations.py test (Bug 1303236)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79058/diff/3-4/

Comment 31

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part 6 - Disable logalloc test;

dlopen support in CFI breaks the test, because the CFI code performs some
memory operations

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

Comment 32

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302891 - Part x - Use newer clang (with dlopen support) (Bug 1303644)

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

Comment 33

[Enes Göktaş [:egoktas]]

Comment on attachment 8791765 [details]
Bug 1302891 - Part 0 - Required user changes to enable CFI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79050/diff/3-4/

Comment 34

[Enes Göktaş [:egoktas]]

Comment on attachment 8791770 [details]
Bug 1302891 - Part 1 - Add --enable-cfi configuration flag;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79060/diff/3-4/

Comment 35

[Enes Göktaş [:egoktas]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79080/diff/4-5/

Comment 36

[Enes Göktaş [:egoktas]]

Comment on attachment 8791766 [details]
Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79052/diff/3-4/

Comment 37

[Enes Göktaş [:egoktas]]

Comment on attachment 8791767 [details]
Bug 1302891 - Part 4 - Disable CFI for binaries that are compiled with -fno-lto;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79054/diff/3-4/

Comment 38

[Enes Göktaş [:egoktas]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79064/diff/5-6/

Comment 39

[Enes Göktaş [:egoktas]]

Comment on attachment 8791768 [details]
Bug 1302891 - Part x - Wrap libxul.so linker to prevent getting the linking process killed (Bug 1303227)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79056/diff/3-4/

Comment 40

[Enes Göktaş [:egoktas]]

Comment on attachment 8791769 [details]
Bug 1302891 - Part x - Skip check_vanilla_allocations.py test (Bug 1303236)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79058/diff/4-5/

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8791765 [details]
Bug 1302891 - Part 0 - Required user changes to enable CFI;

https://reviewboard.mozilla.org/r/79050/#review81642

This is not a patch that is meant to land.

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8791770 [details]
Bug 1302891 - Part 1 - Add --enable-cfi configuration flag;

https://reviewboard.mozilla.org/r/79060/#review81646

Please move most of this in build/autoconf/sanitize.m4

::: old-configure.in:215
(Diff revision 4)
> +    CFLAGS="$CFLAGS -DMOZ_CFI"
> +    AC_DEFINE(MOZ_CFI)

Either you use AC_DEFINE, or you add -DMOZ_CFI to various variables (CPPFLAGS, and possibly others), but not both.

::: old-configure.in:221
(Diff revision 4)
> +    if $CC -Wl,--version 2>&1 | grep -q "GNU gold" ; then
> +        echo "ld is ld.gold"
> +    else
> +        # Try to set ld.gold
> +        LD_GOLD=$(which ld.gold)
> +        if test -n "$LD_GOLD"; then
> +            # Copy ld.gold to ld in the directory where ld.gold resides
> +            LD_LINKER="${LD_GOLD%.*}"
> +            cp "$LD_GOLD" "$LD_LINKER"
> +        else
> +            echo "Could not find ld.gold. Please add path of ld.gold to PATH env."
> +            exit 1
> +        fi
> +
> +        if $CC -Wl,--version 2>&1 | grep -q "GNU gold" ; then
> +            echo "ld is ld.gold"
> +        else
> +            echo "Could not set ld.gold as the linker."
> +            echo "Please ensure that ld is the gold linker (ld.gold)."
> +            exit 1
> +        fi
> +    fi

Just make --enable-cfi imply --enable-gold

::: old-configure.in:245
(Diff revision 4)
> +    LLVM_NM=$(which llvm-nm)
> +    GNU_NM=$(which nm)
> +    if test -n "$LLVM_NM"; then
> +        cp "$LLVM_NM" "$GNU_NM"

On normal systems, that will try to copy llvm-nm to /usr/bin/nm. You don't want that. Even on other systems, you don't want to replace files without the user consenting to it. Why do you need llvm-nm anyways?

::: old-configure.in:254
(Diff revision 4)
> +#    if test -n "$LD_GOLD"; then
> +#        mkdir -p $_objdir/build/unix/gold
> +#        rm -f $_objdir/build/unix/gold/ld
> +#        ln -s "$LD_GOLD" $_objdir/build/unix/gold/ld
> +#        if $CC -B $_objdir/build/unix/gold -Wl,--version 2>&1 | grep -q "GNU gold"; then
> +#            LDFLAGS="$LDFLAGS -B $_objdir/build/unix/gold"
> +#        else
> +#            rm -rf $_objdir/build/unix/gold
> +#        fi
> +#
> +#        # use ld gold for everything
> +#        cp $LD_GOLD /usr/bin/ld
> +#    fi

No need for commented code.

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

https://reviewboard.mozilla.org/r/79080/#review81648

::: build/generate_cfi_whitelist.py:7
(Diff revision 5)
> +def extractInterfaces(idlFileName):
> +  interfaceNames = set()
> +  with open(idlFileName, "rb") as f:
> +    for l in f.xreadlines():
> +      if l.startswith("interface "):
> +        spl = l.split(":")
> +        if len(spl) == 1: continue
> +
> +        interfaceName = spl[0].split()[1]
> +        interfaceNames.add(interfaceName)
> +
> +  return interfaceNames

We have a python idl parser (import xpidl).

::: build/generate_cfi_whitelist.py:24
(Diff revision 5)
> +  cmd = 'find ' + topsrcdir + ' -type f -name *.idl'
> +  proc = subprocess.Popen(cmd.split(), stdout=subprocess.PIPE)

Instead of doing cmd = ... ; Popen(cmd.split()), you should do cmd = ['find', topsrcdir, '-type', 'f', ...]; Popen(cmd).

That being said, there are python APIs to scan directories (e.g. os.walk), and you should use that instead of spawning a `find` process.

That being said, the build system knows about all the idl files, so there is actually no need to scan the whole tree to find them. We however don't have them handy when processing GENERATED_FILES. Ask gps for guidance on this, he has better knowledge of that part of the build system than I have.

::: old-configure.in:267
(Diff revision 5)
> +     echo "Create cfi-whitelist.txt"
> +     $_topsrcdir/build/generate_cfi_whitelist.py
> +     if test -f "$_topsrcdir/cfi-whitelist.txt"; then

You don't want to generate files in the source directory.

Use GENERATED_FILES in some moz.build instead.

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

https://reviewboard.mozilla.org/r/79080/#review81658

Comment 45

[Mike Hommey [:glandium]]

Comment on attachment 8791767 [details]
Bug 1302891 - Part 4 - Disable CFI for binaries that are compiled with -fno-lto;

https://reviewboard.mozilla.org/r/79054/#review81660

Comment 46

[Mike Hommey [:glandium]]

Comment on attachment 8791778 [details]
Bug 1302891 - Part 5 - Make __cfi_check a global symbol;

https://reviewboard.mozilla.org/r/79064/#review81662

::: build/unix/gnu-ld-scripts/components-version-script:6
(Diff revision 6)
>  EXPORTED {
>     global:
>  		NSModule;
>  		NSGetModule;
>  		__RLD_MAP;
> +		__cfi_check;

Considering you're folding binary components to make cfi work, you shouldn't be using this file.

::: db/sqlite3/src/sqlite.symbols:160
(Diff revision 6)
>  sqlite3_vmprintf
>  #ifdef DEBUG
>  sqlite3_mutex_held
>  sqlite3_mutex_notheld
>  #endif
> +__cfi_check

I think there should be a #ifdef something here (MOZ_CFI?)

Comment 47

[Mike Hommey [:glandium]]

Comment on attachment 8795999 [details]
Bug 1302891 - Part 6 - Disable logalloc test;

https://reviewboard.mozilla.org/r/81972/#review81664

There is a reason this test exists, and disabling it unconditionally is not an option. That being said, it failing in your builds is most likely unrelated to CFI. Please file a separate bug with the error message you see (which, if that's what I think it is, is about an additional 70+kB allocation happening at process initialization, and is due to libstdc++).

Comment 48

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8791766 [details]
Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

https://reviewboard.mozilla.org/r/79052/#review81822

Comment 49

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mhentges, could you have a look please?
For more information, please visit auto_nag documentation.