Open Bug 1302891 Opened 4 years ago Updated 2 years ago

Add Clang CFI support to build system

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: egoktas, Assigned: egoktas)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
jld
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
glandium
: review-
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
glandium
: review-
Details
58 bytes, text/x-review-board-request
Details
Add -p (linux64-cfi) option that will build Firefox with CFI in Clang on Linux x64.
Summary: Create taskcluster task to build firefox with CFI in Clang on Linux64 → Add --enable-cfi flag that will build Firefox with Clang + CFI
Instead of adding the linux64-cfi, we will add a
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.
Summary: Add --enable-cfi flag that will build Firefox with Clang + CFI → Add Clang CFI support to build system
Depends on: 1302855
Depends on: 1303227
Depends on: 1303236
Depends on: 1303644
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.
Attachment #8791765 - Flags: review?(mh+mozilla)
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.
Attachment #8791770 - Flags: review?(mh+mozilla) → review-
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 on attachment 8791821 [details]
Bug 1302891 - Part 2 - Create CFI vtable whitelist during configuration;

https://reviewboard.mozilla.org/r/79080/#review81658
Attachment #8791821 - Flags: review?(mh+mozilla)
Attachment #8791766 - Flags: review?(mh+mozilla) → review?(jld)
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
Attachment #8791767 - Flags: review?(mh+mozilla) → review+
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?)
Attachment #8791778 - Flags: review?(mh+mozilla)
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++).
Attachment #8795999 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8791766 [details]
Bug 1302891 - Part 3 - Whitelist mremap in Sandbox;

https://reviewboard.mozilla.org/r/79052/#review81822
Attachment #8791766 - Flags: review?(jld) → review+
Blocks: cfi
Product: Firefox → Core
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.