Closed Bug 1403346 Opened 2 years ago Closed 2 years ago

Move remaining compile flags from config.mk to moz.build computed flags

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 1 open bug)

Details

Attachments

(20 files)

59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
No description provided.
Attachment #8915292 - Flags: review?(mh+mozilla)
Comment on attachment 8915278 [details]
Bug 1403346 - Use AC_SUBST_LIST for DSO_CFLAGS and DSO_PIC_CFLAGS

https://reviewboard.mozilla.org/r/184136/#review192992
Attachment #8915278 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915279 [details]
Bug 1403346 - Move DSO_CFLAGS and DSO_PIC_CFLAGS to computed flags.

https://reviewboard.mozilla.org/r/184138/#review192998
Attachment #8915279 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915280 [details]
Bug 1403346 - Move RTL_FLAGS to computed flags.

https://reviewboard.mozilla.org/r/184140/#review193000

::: python/mozbuild/mozbuild/compilation/database.py:79
(Diff revision 1)
>                  self._build_db_line(obj.objdir, obj.relativedir, obj.config, f,
>                                      obj.canonical_suffix)
>  
>          elif isinstance(obj, VariablePassthru):
>              for var in ('MOZBUILD_CFLAGS', 'MOZBUILD_CXXFLAGS',
> -                        'MOZBUILD_CMFLAGS', 'MOZBUILD_CMMFLAGS',
> +                        'MOZBUILD_CMFLAGS', 'MOZBUILD_CMMFLAGS',):

you can remove the last comma

::: python/mozbuild/mozbuild/frontend/emitter.py:975
(Diff revision 1)
>              rtl_flag = '-MT' if use_static_lib else '-MD'
>              if (context.config.substs.get('MOZ_DEBUG') and
>                      not context.config.substs.get('MOZ_NO_DEBUG_RTL')):
>                  rtl_flag += 'd'
>              # Use a list, like MOZBUILD_*FLAGS variables
>              passthru.variables['RTL_FLAGS'] = [rtl_flag]

shouldn't the passthru be removed?
Attachment #8915280 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915281 [details]
Bug 1403346 - Move OS_COMPILE flags to computed flags.

https://reviewboard.mozilla.org/r/184142/#review193002

::: python/mozbuild/mozbuild/frontend/context.py:20
(Diff revision 1)
>  """
>  
>  from __future__ import absolute_import, unicode_literals
>  
>  import os
> +import shlex

don't use shlex. Use mozbuild.shellutil.split.

::: python/mozbuild/mozbuild/frontend/context.py:315
(Diff revision 1)
> +            'topobjdir': context.config.topobjdir,
> +        }
> +        def normalize_var(var):
> +            val = context.config.substs.get(var)
> +            if val:
> +                val = expand_variables(val, global_vars)

On the long term, I think we should move away from "make expansions" in variables at the configure level, by not using them in the first place. File a followup?

::: python/mozbuild/mozbuild/frontend/context.py:316
(Diff revision 1)
> +        }
> +        def normalize_var(var):
> +            val = context.config.substs.get(var)
> +            if val:
> +                val = expand_variables(val, global_vars)
> +                return shlex.split(val)

how about turning OS_COMPILE_FLAGS into an AC_SUBST_LIST (and using a shell-aware split in config.status for AC_SUBST_LISTs, instead of plain split)?
Attachment #8915281 - Flags: review?(mh+mozilla)
Comment on attachment 8915282 [details]
Bug 1403346 - Make a separate variable used to append pgo flags to compile command lines.

https://reviewboard.mozilla.org/r/184144/#review193008
Attachment #8915282 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915283 [details]
Bug 1403346 - Use AC_SUBST_LIST for various configure variables containing compile flags.

https://reviewboard.mozilla.org/r/184146/#review193010

Yeah.... we really need a shell-aware split for AC_SUBST_LIST before this...
Attachment #8915283 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915284 [details]
Bug 1403346 - Move C{XX}FLAGS to mozbuild computed flags.

https://reviewboard.mozilla.org/r/184148/#review193012

::: js/src/build/Makefile.in:32
(Diff revision 1)
>  # combinations, but is worth running by hand every once in a while.
>  check-vanilla-allocations-aggressive:
>  	$(PYTHON) $(topsrcdir)/config/check_vanilla_allocations.py --aggressive $(REAL_LIBRARY)
>  
>  ifeq ($(OS_ARCH),Linux)
> -ifeq (,$(filter -flto,$(CFLAGS) $(CXXFLAGS) $(LDFLAGS)))
> +ifeq (,$(filter -flto,$(OS_CFLAGS) $(OS_CXXFLAGS) $(LDFLAGS)))

you want COMPUTED, here.

::: python/mozbuild/mozbuild/frontend/context.py:343
(Diff revision 1)
>               ('CXXFLAGS', 'CFLAGS')),
>              ('RTL', None, ('CXXFLAGS', 'CFLAGS')),
>              ('OS_COMPILE_CFLAGS', normalize_var('OS_COMPILE_CFLAGS'), ('CFLAGS',)),
>              ('OS_COMPILE_CXXFLAGS', normalize_var('OS_COMPILE_CXXFLAGS'), ('CXXFLAGS',)),
> +            ('OS_CPPFLAGS', normalize_var('OS_CPPFLAGS'),
> +             ('CXXFLAGS', 'CFLAGS', 'LD_CXXFLAGS', 'LD_CFLAGS')),

CXX_LDFLAGS/C_LDFLAGS would be better.

::: python/mozbuild/mozbuild/frontend/context.py:371
(Diff revision 1)
> +        if self._context.config.substs.get('MOZ_PGO'):
> +            optimize_flags = self._context.config.substs.get('MOZ_PGO_OPTIMIZE_FLAGS')
> +            # If MOZ_PGO_OPTIMIZE_FLAGS is empty we fall back to MOZ_OPTIMIZE_FLAGS.
> +            # Presently this occurs on Windows.
> +            if not optimize_flags:
> +                optimize_flags = self._context.config.substs.get('MOZ_OPTIMIZE_FLAGS')
> +        else:
> +            optimize_flags = self._context.config.substs.get('MOZ_OPTIMIZE_FLAGS')

DRY version:

optimize_flags = None
if self._context.config.substs.get('MOZ_PGO'):
   optimize_flags = ...
if not optimize_flags:
   optimize_flags = self._context.config.substs.get('MOZ_OPTIMIZE_FLAGS')
Attachment #8915284 - Flags: review?(mh+mozilla)
Comment on attachment 8915285 [details]
Bug 1403346 - Implement ALLOW_COMPILER_WARNINGS as a template.

https://reviewboard.mozilla.org/r/184150/#review193016
Attachment #8915285 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915286 [details]
Bug 1403346 - Replace all uses of ALLOW_COMPILER_WARNINGS with a template, remove ALLOW_COMPILER_WARNINGS.

https://reviewboard.mozilla.org/r/184152/#review193018
Attachment #8915286 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915287 [details]
Bug 1403346 - Convert sse flags filtering in browser/app/Makefile.in to mozbuild.

https://reviewboard.mozilla.org/r/185812/#review193020

::: browser/app/moz.build:119
(Diff revision 1)
>  
>  if CONFIG['MOZ_LINUX_32_SSE2_STARTUP_ERROR']:
>      DEFINES['MOZ_LINUX_32_SSE2_STARTUP_ERROR'] = True
> +    COMPILE_FLAGS['OS_CXXFLAGS'] = [
> +        f for f in COMPILE_FLAGS.get('OS_CXXFLAGS', [])
> +        if not f.startswith('-march') and f not in ('-msse', '-msse2', '-mfpmath=sse')

-march=
Attachment #8915287 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915288 [details]
Bug 1403346 - Move cxxflags filtering for libfuzzer from Makefile.in to moz.build

https://reviewboard.mozilla.org/r/185814/#review193022

::: tools/fuzzing/libfuzzer/moz.build:45
(Diff revision 1)
> +for flags_var in ('OS_CFLAGS', 'OS_CXXFLAGS'):
> +    COMPILE_FLAGS[flags_var] = [
> +        f for f in COMPILE_FLAGS.get(flags_var, [])
> +        if not f.startswith('-fsanitize')
> +    ]

maybe we should have a helper for this?
Attachment #8915288 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915289 [details]
Bug 1403346 - Define flags loading the clang plugin in configure rather than the make backend.

https://reviewboard.mozilla.org/r/186498/#review193024

::: build/autoconf/clang-plugin.m4:158
(Diff revision 1)
>          ])
>      if test "$ac_cv_has_accepts_ignoringParenImpCasts" = "yes"; then
>        LLVM_CXXFLAGS="$LLVM_CXXFLAGS -DHAS_ACCEPTS_IGNORINGPARENIMPCASTS"
>      fi
>  
> +    CLANG_PLUGIN='$(topobjdir)'/build/clang-plugin/${DLL_PREFIX}clang-plugin${DLL_SUFFIX}

just use $_objdir or $MOZ_BUILD_ROOT, expanding it here and now.
Attachment #8915289 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915290 [details]
Bug 1403346 - Implement clang-plugin cxxflags in moz.build.

https://reviewboard.mozilla.org/r/186500/#review193026

::: build/clang-plugin/moz.build:71
(Diff revision 1)
> +if CONFIG['HOST_OS_ARCH'] == 'WINNT':
> +    extra_cxxflags = ['-GR-', '-EHsc']
> +else:
> +    extra_cxxflags = ['-fno-rtti', '-fno-exceptions']
> +
> +COMPILE_FLAGS['OS_CXXFLAGS'] = CONFIG['LLVM_CXXFLAGS'].split() + extra_cxxflags

should AC_SUBST_LIST LLVM_CXXFLAGS instead.

::: build/clang-plugin/tests/moz.build:54
(Diff revision 1)
>  NoVisibilityFlags()
> +
> +# Build without any warning flags, and with clang verify flag for a
> +# syntax-only build (no codegen), without a limit on the number of errors.
> +COMPILE_FLAGS['OS_CXXFLAGS'] = (
> +    [f for f in COMPILE_FLAGS.get('OS_CXXFLAGS', []) if f[:2] != '-W'] +

use .startswith. It's longer, but easier to reason about.
Attachment #8915290 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review193028

::: build/unix/elfhack/inject/moz.build:27
(Diff revision 1)
> +if '-idirafter' in COMPILE_FLAGS.get('OS_CPPFLAGS', []):
> +    idx = COMPILE_FLAGS['OS_CPPFLAGS'].index('-idirafter')
> +    idirafter = [''.join(COMPILE_FLAGS['OS_CPPFLAGS'][idx:idx + 2])]
> +COMPILE_FLAGS['OS_CPPFLAGS'] = ['-O2', '-fno-stack-protector'] + idirafter + [
> +    f for f in COMPILE_FLAGS.get('OS_CPPFLAGS', []) if f[:2] in ('-m', '-I')
> +]

This misses half the trick with idirafter, which is that we can have -idirafterfoo without a space as input (yes, that's valid).

::: build/unix/elfhack/inject/moz.build:35
(Diff revision 1)
> +COMPILE_FLAGS['OS_CPPFLAGS'] = ['-O2', '-fno-stack-protector'] + idirafter + [
> +    f for f in COMPILE_FLAGS.get('OS_CPPFLAGS', []) if f[:2] in ('-m', '-I')
> +]
> +for v in ('OS_CFLAGS', 'DEBUG', 'CLANG_PLUGIN', 'OPTIMIZE', 'FRAMEPTR'):
> +    COMPILE_FLAGS[v] = [f for f in COMPILE_FLAGS.get(v, [])
> +                        if f[:2] in ('-m', '-I')]

use .startswith (you can pass it a tuple)
Attachment #8915291 - Flags: review?(mh+mozilla)
Comment on attachment 8915292 [details]
Bug 1403346 - Remove clang plugin flags from stdc++compat through moz.build rather than Makefile.in

https://reviewboard.mozilla.org/r/186504/#review193030
Attachment #8915292 - Flags: review?(mh+mozilla) → review+
Blocks: 1256604
Comment on attachment 8915280 [details]
Bug 1403346 - Move RTL_FLAGS to computed flags.

https://reviewboard.mozilla.org/r/184140/#review193000

> shouldn't the passthru be removed?

Host flags still need this for the moment.
Comment on attachment 8915281 [details]
Bug 1403346 - Move OS_COMPILE flags to computed flags.

https://reviewboard.mozilla.org/r/184142/#review193002

> On the long term, I think we should move away from "make expansions" in variables at the configure level, by not using them in the first place. File a followup?

Filed bug 1407421
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review193028

> This misses half the trick with idirafter, which is that we can have -idirafterfoo without a space as input (yes, that's valid).

I re-implemented this here without a space, with `''.join(COMPILE_FLAGS[...` instead of `' '.join(...` or just the raw slice to get an identical command line for comparison purposes.
Comment on attachment 8917550 [details]
Bug 1403346 - Expand references to the objdir in old-configure.

https://reviewboard.mozilla.org/r/188202/#review193900

::: js/src/old-configure.in:487
(Diff revision 1)
>      LDFLAGS="$LDFLAGS -Wl,--build-id"
>      AC_TRY_LINK(,,AC_MSG_RESULT([yes]),
>                    AC_MSG_RESULT([no])
>                    LDFLAGS=$_SAVE_LDFLAGS)
>  
> -    _DEFINES_CFLAGS='-include $(topobjdir)/js/src/js-confdefs.h -DMOZILLA_CLIENT'
> +    _DEFINES_CFLAGS="-include $ROOT_OBJDIR/js/src/js-confdefs.h -DMOZILLA_CLIENT"

I'd rather use _objdir or MOZ_BUILD_ROOT here, without adding js/src, since it's already in there, than add $ROOT_OBJDIR just for this.
Attachment #8917550 - Flags: review?(mh+mozilla)
Comment on attachment 8917549 [details]
Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds.

https://reviewboard.mozilla.org/r/188200/#review193902
Attachment #8917549 - Flags: review?(mh+mozilla)
Comment on attachment 8917551 [details]
Bug 1403346 - Use AC_SUBST_LIST for OS_COMPILE_C{XX}FLAGS.

https://reviewboard.mozilla.org/r/188204/#review193904
Attachment #8917551 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8917552 [details]
Bug 1403346 - Use mozbuild.shellutil.split instead of split in config.status

https://reviewboard.mozilla.org/r/188206/#review193906

::: old-configure.in:4137
(Diff revision 1)
>  dnl = Build depencency options
>  dnl =
>  dnl ========================================================
>  MOZ_ARG_HEADER(Build dependencies)
>  
> +if test "$COMPILE_ENVIRONMENT"; then

this doesn't seem related.
Attachment #8917552 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915281 [details]
Bug 1403346 - Move OS_COMPILE flags to computed flags.

https://reviewboard.mozilla.org/r/184142/#review193908

::: python/mozbuild/mozbuild/frontend/context.py:26
(Diff revision 2)
>  from collections import (
>      Counter,
>      OrderedDict,
>  )
>  from mozbuild.util import (
> +    expand_variables,

you're not using this anymore

::: python/mozbuild/mozbuild/frontend/context.py:41
(Diff revision 2)
>      StrictOrderingOnAppendListWithAction,
>      StrictOrderingOnAppendListWithFlagsFactory,
>      TypedList,
>      TypedNamedTuple,
>  )
> +from mozbuild import shellutil

same here
Attachment #8915281 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915284 [details]
Bug 1403346 - Move C{XX}FLAGS to mozbuild computed flags.

https://reviewboard.mozilla.org/r/184148/#review193910
Attachment #8915284 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915289 [details]
Bug 1403346 - Define flags loading the clang plugin in configure rather than the make backend.

https://reviewboard.mozilla.org/r/186498/#review193912

::: build/autoconf/clang-plugin.m4:158
(Diff revision 2)
>          ])
>      if test "$ac_cv_has_accepts_ignoringParenImpCasts" = "yes"; then
>        LLVM_CXXFLAGS="$LLVM_CXXFLAGS -DHAS_ACCEPTS_IGNORINGPARENIMPCASTS"
>      fi
>  
> +    CLANG_PLUGIN=$ROOT_OBJDIR/build/clang-plugin/${DLL_PREFIX}clang-plugin${DLL_SUFFIX}

I guess you need root_objdir here... but I don't really like to add it just for this...

I'd rather set CLANG_PLUGIN from python configure.
Attachment #8915289 - Flags: review+
Comment on attachment 8917553 [details]
Bug 1403346 - Use AC_SUBST_LIST for LLVM_CXXFLAGS.

https://reviewboard.mozilla.org/r/188472/#review193914
Attachment #8917553 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915290 [details]
Bug 1403346 - Implement clang-plugin cxxflags in moz.build.

https://reviewboard.mozilla.org/r/186500/#review193924

::: build/clang-plugin/moz.build:71
(Diff revisions 1 - 2)
>  if CONFIG['HOST_OS_ARCH'] == 'WINNT':
>      extra_cxxflags = ['-GR-', '-EHsc']
>  else:
>      extra_cxxflags = ['-fno-rtti', '-fno-exceptions']
>  
> -COMPILE_FLAGS['OS_CXXFLAGS'] = CONFIG['LLVM_CXXFLAGS'].split() + extra_cxxflags
> +COMPILE_FLAGS['OS_CXXFLAGS'] = CONFIG.get('LLVM_CXXFLAGS', []) + extra_cxxflags

It being an AC_SUBST_LIST should guarantee that it's always defined as a list, empty in there's no value. So CONFIG['LLVM_CXXFLAGS'] should work.
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review193930

::: build/unix/elfhack/inject/moz.build:27
(Diff revision 2)
>  ]
>  
>  NO_PGO = True
>  
> +idirafter = []
> +if '-idirafter' in COMPILE_FLAGS.get('OS_CPPFLAGS', []):

same as for LLVM_CXXFLAGS.

::: build/unix/elfhack/inject/moz.build:28
(Diff revision 2)
> +    idx = COMPILE_FLAGS['OS_CPPFLAGS'].index('-idirafter')
> +    idirafter = [''.join(COMPILE_FLAGS['OS_CPPFLAGS'][idx:idx + 2])]

there could be multiple idirafters.

::: build/unix/elfhack/inject/moz.build:33
(Diff revision 2)
> +    idx = COMPILE_FLAGS['OS_CPPFLAGS'].index('-idirafter')
> +    idirafter = [''.join(COMPILE_FLAGS['OS_CPPFLAGS'][idx:idx + 2])]
> +COMPILE_FLAGS['OS_CPPFLAGS'] = ['-O2', '-fno-stack-protector'] + idirafter + [
> +    f for f in COMPILE_FLAGS.get('OS_CPPFLAGS', []) if f.startswith(('-m', '-I'))
> +]
> +for v in ('OS_CFLAGS', 'DEBUG', 'CLANG_PLUGIN', 'OPTIMIZE', 'FRAMEPTR'):

considering how CFLAGS are usually abused (more than those other variables), it would be better to do the idirafter treatment on CFLAGS too. But you might as well apply it on all variables, since you're filtering -m and -I for all of them anyways.

::: build/unix/elfhack/moz.build:30
(Diff revision 2)
> +COMPILE_FLAGS['OS_CXXFLAGS'] = [
> +    f for f in COMPILE_FLAGS['OS_CXXFLAGS'] if f != '-fno-exceptions'
> +] + ['-fexceptions']

We may want, at some point, to allow something like
COMPILE_FLAGS['OS_CXXFLAGS'] -= ['-fno-exceptions']
Attachment #8915291 - Flags: review?(mh+mozilla)
Comment on attachment 8917550 [details]
Bug 1403346 - Expand references to the objdir in old-configure.

https://reviewboard.mozilla.org/r/188202/#review193900

> I'd rather use _objdir or MOZ_BUILD_ROOT here, without adding js/src, since it's already in there, than add $ROOT_OBJDIR just for this.

I tried this initially, but it fails on spidermonkey builds, which I guess end up with a topobjdir as though they're a browser build due to how they run configure.
Comment on attachment 8915289 [details]
Bug 1403346 - Define flags loading the clang plugin in configure rather than the make backend.

https://reviewboard.mozilla.org/r/186498/#review196144

::: moz.configure:283
(Diff revision 3)
>          not (pgo and target.os == 'WINNT')):
>          return True
>  
>  set_config('LINK_GTEST_DURING_COMPILE', build_gtest)
>  
> +# clang-plugin location

This should go in toolchain.configure

::: moz.configure:285
(Diff revision 3)
>  
>  set_config('LINK_GTEST_DURING_COMPILE', build_gtest)
>  
> +# clang-plugin location
> +# ==============================================================
> +@depends(library_name_info, check_build_environment)

host_library_name_info

::: moz.configure:296
(Diff revision 3)
> +        os.path.join(topobjdir, 'build', 'clang-plugin',
> +                     '%sclang-plugin%s' % (library_name_info.dll.prefix,
> +                                           library_name_info.dll.suffix))
> +    )
> +
> +add_old_configure_assignment('CLANG_PLUGIN', clang_plugin_path)

You could move --enable-clang-plugin (the option only), and make the whole block depend on it.
Attachment #8915289 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8917550 [details]
Bug 1403346 - Expand references to the objdir in old-configure.

https://reviewboard.mozilla.org/r/188202/#review196146
Attachment #8917550 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review196152

::: build/unix/elfhack/inject/moz.build:30
(Diff revision 3)
> +    for flag in COMPILE_FLAGS[v]:
> +        if flag == '-idirafter':
> +            idirafter.append(''.join(COMPILE_FLAGS[v][idx:idx + 2]))
> +        idx += 1
> +
> +    COMPILE_FLAGS[v] = idirafter + [f for f in COMPILE_FLAGS.get(v, [])
> +                                    if f.startswith(('-m', '-I'))]

I know this is unlikely, but if there's a mix of -idirafter foo and -idirafter foo, you end up reordering them. It would be better to create a fresh list from scratch, by appending flags to it in the same order as they come in.
Attachment #8915291 - Flags: review?(mh+mozilla)
Comment on attachment 8917549 [details]
Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds.

https://reviewboard.mozilla.org/r/188200/#review196154

::: commit-message-964a3:1
(Diff revision 2)
> +Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds.
> +
> +A future commit will make it an error to output a variable from configure
> +using AC_SUBST_LIST that contains a make variable reference, which
> +coincidentally only occurs during artifact build when setting variables that
> +aren't needed there anyway.

This is not what the patch does at all anymore.
Attachment #8917549 - Flags: review?(mh+mozilla)
Comment on attachment 8917549 [details]
Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds.

https://reviewboard.mozilla.org/r/188200/#review196154

> This is not what the patch does at all anymore.

I'd say it is, but I'll see if I can clarify the commit message.
Comment on attachment 8915289 [details]
Bug 1403346 - Define flags loading the clang plugin in configure rather than the make backend.

https://reviewboard.mozilla.org/r/186498/#review196144

> This should go in toolchain.configure

The library prefix/suffix and some related stuff come after including toolchain.configure, apparently so we can figure these out during artifact builds.

> host_library_name_info

Oddly enough the clang plugin is built as ".dylib" on mac-cross builds currently.
Comment on attachment 8917549 [details]
Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds.

https://reviewboard.mozilla.org/r/188200/#review197520
Attachment #8917549 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review197522

::: build/unix/elfhack/inject/moz.build:29
(Diff revision 4)
> +    idx = 0
> +    for flag in COMPILE_FLAGS[v]:

maybe we should expose enumerate() to the moz.build sandbox?

::: build/unix/elfhack/inject/moz.build:33
(Diff revision 4)
> +    flags = []
> +    idx = 0
> +    for flag in COMPILE_FLAGS[v]:
> +        if flag == '-idirafter':
> +            flags.append(''.join(COMPILE_FLAGS[v][idx:idx + 2]))
> +        elif flag.startswith(('-m', '-I')):

you need -idirafter here too

::: build/unix/elfhack/inject/moz.build:38
(Diff revision 4)
> +        elif flag.startswith(('-m', '-I')):
> +            flags.append(flag)
> +        idx += 1
> +    COMPILE_FLAGS[v] = flags
> +
> +COMPILE_FLAGS['OS_CPPFLAGS'] = ['-O2', '-fno-stack-protector'] + COMPILE_FLAGS['OS_CPPFLAGS']

CFLAGS
Attachment #8915291 - Flags: review?(mh+mozilla)
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review197522

> maybe we should expose enumerate() to the moz.build sandbox?

We could... I'm a little wary of making it easier to do more involved things in moz.build, this might be better off remaining a one off.

> CFLAGS

CFLAGS as the make backend knows it starts off with OS_CPPFLAGS, so this seems like the straightforward way to end up emitting the flags we had before.
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review197938

::: build/unix/elfhack/inject/moz.build:38
(Diff revision 5)
> +        elif flag.startswith(('-m', '-I', '-idirafter')):
> +            flags.append(flag)
> +        idx += 1
> +    COMPILE_FLAGS[v] = flags
> +
> +COMPILE_FLAGS['OS_CPPFLAGS'] = ['-O2', '-fno-stack-protector'] + COMPILE_FLAGS['OS_CPPFLAGS']

It doesn't matter if -O2 and -fno-stack-protector are not the first arguments, reproducing the current command line. CPPFLAGS and CFLAGS are filtered, the only remaining stuff will be -m* -I* and -idirafter*, so no option that might conflict with them. Which, by the way, is why the current Makefile works, because otherwise, those flags should be enforced to come *last*, actually, otherwise, we could end up with e.g. -fstack-protector overriding -fno-stack-protector.

Anyways, since we don't actually need to care about the order, we might as well place compiler flags in the variable for compiler flags, not the one for preprocessor flags.
Attachment #8915291 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915291 [details]
Bug 1403346 - Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS

https://reviewboard.mozilla.org/r/186502/#review197938

> It doesn't matter if -O2 and -fno-stack-protector are not the first arguments, reproducing the current command line. CPPFLAGS and CFLAGS are filtered, the only remaining stuff will be -m* -I* and -idirafter*, so no option that might conflict with them. Which, by the way, is why the current Makefile works, because otherwise, those flags should be enforced to come *last*, actually, otherwise, we could end up with e.g. -fstack-protector overriding -fno-stack-protector.
> 
> Anyways, since we don't actually need to care about the order, we might as well place compiler flags in the variable for compiler flags, not the one for preprocessor flags.

Ok
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6b4027ddd753 -d c9d85d930e9c: rebasing 429728:6b4027ddd753 "Bug 1403346 - Use AC_SUBST_LIST for DSO_CFLAGS and DSO_PIC_CFLAGS r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429729:16cbb1cfb934 "Bug 1403346 - Move DSO_CFLAGS and DSO_PIC_CFLAGS to computed flags. r=glandium"
rebasing 429730:2e9deca11da6 "Bug 1403346 - Move RTL_FLAGS to computed flags. r=glandium"
rebasing 429731:1e09d04964dc "Bug 1403346 - Expand references to the objdir in old-configure. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429732:799a38ce77d6 "Bug 1403346 - Use AC_SUBST_LIST for OS_COMPILE_C{XX}FLAGS. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429733:7c18e752c68e "Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds. r=glandium"
merging old-configure.in
rebasing 429734:ecaa94d2d271 "Bug 1403346 - Use mozbuild.shellutil.split instead of split in config.status r=glandium"
merging build/moz.configure/old.configure
rebasing 429735:2aed0a6cd767 "Bug 1403346 - Move OS_COMPILE flags to computed flags. r=glandium"
rebasing 429736:462f380996f9 "Bug 1403346 - Make a separate variable used to append pgo flags to compile command lines. r=glandium"
merging config/rules.mk
merging js/src/old-configure.in
merging old-configure.in
rebasing 429737:db59ea5a409a "Bug 1403346 - Use AC_SUBST_LIST for various configure variables containing compile flags. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
warning: conflicts while merging js/src/old-configure.in! (edit, then use 'hg resolve --mark')
warning: conflicts while merging old-configure.in! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 41f9a0447518 -d 4a271421158a: rebasing 429729:41f9a0447518 "Bug 1403346 - Use AC_SUBST_LIST for DSO_CFLAGS and DSO_PIC_CFLAGS r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429730:92a963c07668 "Bug 1403346 - Move DSO_CFLAGS and DSO_PIC_CFLAGS to computed flags. r=glandium"
rebasing 429731:2c704d26a997 "Bug 1403346 - Move RTL_FLAGS to computed flags. r=glandium"
rebasing 429732:d7b00a65ed7c "Bug 1403346 - Expand references to the objdir in old-configure. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429733:97d54c9dd96d "Bug 1403346 - Use AC_SUBST_LIST for OS_COMPILE_C{XX}FLAGS. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
rebasing 429734:c5bf6808e047 "Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds. r=glandium"
merging old-configure.in
rebasing 429735:514503e2e46c "Bug 1403346 - Use mozbuild.shellutil.split instead of split in config.status r=glandium"
merging build/moz.configure/old.configure
rebasing 429736:d3981d8e1c22 "Bug 1403346 - Move OS_COMPILE flags to computed flags. r=glandium"
rebasing 429737:e86b0faa6299 "Bug 1403346 - Make a separate variable used to append pgo flags to compile command lines. r=glandium"
merging config/rules.mk
merging js/src/old-configure.in
merging old-configure.in
rebasing 429738:59f9096f8f2a "Bug 1403346 - Use AC_SUBST_LIST for various configure variables containing compile flags. r=glandium"
merging js/src/old-configure.in
merging old-configure.in
warning: conflicts while merging js/src/old-configure.in! (edit, then use 'hg resolve --mark')
warning: conflicts while merging old-configure.in! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da833838314c
Use AC_SUBST_LIST for DSO_CFLAGS and DSO_PIC_CFLAGS r=glandium
https://hg.mozilla.org/integration/autoland/rev/b5070fa42ee6
Move DSO_CFLAGS and DSO_PIC_CFLAGS to computed flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/0bc331406b77
Move RTL_FLAGS to computed flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/76cbb735d471
Expand references to the objdir in old-configure. r=glandium
https://hg.mozilla.org/integration/autoland/rev/80674bf1fef5
Use AC_SUBST_LIST for OS_COMPILE_C{XX}FLAGS. r=glandium
https://hg.mozilla.org/integration/autoland/rev/f675216f052e
Do not set unneeded CFLAGS variables during artifact builds. r=glandium
https://hg.mozilla.org/integration/autoland/rev/e20f0eb3eda3
Use mozbuild.shellutil.split instead of split in config.status r=glandium
https://hg.mozilla.org/integration/autoland/rev/6c9ead962e8e
Move OS_COMPILE flags to computed flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/2b8f53e41e4e
Make a separate variable used to append pgo flags to compile command lines. r=glandium
https://hg.mozilla.org/integration/autoland/rev/f11039e7ba7a
Use AC_SUBST_LIST for various configure variables containing compile flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/3206a73af545
Move C{XX}FLAGS to mozbuild computed flags. r=glandium
https://hg.mozilla.org/integration/autoland/rev/0c470da05f01
Implement ALLOW_COMPILER_WARNINGS as a template. r=glandium
https://hg.mozilla.org/integration/autoland/rev/5f700fe50260
Replace all uses of ALLOW_COMPILER_WARNINGS with a template, remove ALLOW_COMPILER_WARNINGS. r=glandium
https://hg.mozilla.org/integration/autoland/rev/258a6a41c365
Convert sse flags filtering in browser/app/Makefile.in to mozbuild. r=glandium
https://hg.mozilla.org/integration/autoland/rev/ffb3835d5f55
Move cxxflags filtering for libfuzzer from Makefile.in to moz.build r=glandium
https://hg.mozilla.org/integration/autoland/rev/c2886f830793
Define flags loading the clang plugin in configure rather than the make backend. r=glandium
https://hg.mozilla.org/integration/autoland/rev/1090ab599504
Use AC_SUBST_LIST for LLVM_CXXFLAGS. r=glandium
https://hg.mozilla.org/integration/autoland/rev/b9be371d987a
Implement clang-plugin cxxflags in moz.build. r=glandium
https://hg.mozilla.org/integration/autoland/rev/57030dd6ffd8
Implement cflags filtering for elfhack in mozbuild COMPILE_FLAGS r=glandium
https://hg.mozilla.org/integration/autoland/rev/da4aaa3d6c59
Remove clang plugin flags from stdc++compat through moz.build rather than Makefile.in r=glandium
Depends on: 1412057
Depends on: 1412267
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.