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

Comment on attachment 8915279 [details] Bug 1403346 - Move DSO_CFLAGS and DSO_PIC_CFLAGS to computed flags. https://reviewboard.mozilla.org/r/184138/#review192998

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?

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)?

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

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...

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')

Comment on attachment 8915285 [details] Bug 1403346 - Implement ALLOW_COMPILER_WARNINGS as a template. https://reviewboard.mozilla.org/r/184150/#review193016

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

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=

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?

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.

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.

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)

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

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.

Comment on attachment 8917549 [details] Bug 1403346 - Do not set unneeded CFLAGS variables during artifact builds. https://reviewboard.mozilla.org/r/188200/#review193902

Comment on attachment 8917551 [details] Bug 1403346 - Use AC_SUBST_LIST for OS_COMPILE_C{XX}FLAGS. https://reviewboard.mozilla.org/r/188204/#review193904

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.

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

Comment on attachment 8915284 [details] Bug 1403346 - Move C{XX}FLAGS to mozbuild computed flags. https://reviewboard.mozilla.org/r/184148/#review193910

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.

Comment on attachment 8917553 [details] Bug 1403346 - Use AC_SUBST_LIST for LLVM_CXXFLAGS. https://reviewboard.mozilla.org/r/188472/#review193914

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']

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.

Comment on attachment 8917550 [details] Bug 1403346 - Expand references to the objdir in old-configure. https://reviewboard.mozilla.org/r/188202/#review196146

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.

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.

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

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

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.

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)