Closed Bug 1411712 Opened 7 years ago Closed 7 years ago

Move ldflags to moz.build

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 1 open bug)

Details

Attachments

(10 files)

Bug 1411712 - Make a new variable for pgo flags added to LDFLAGS.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Use AC_SUBST_LIST instead of AC_SUBST for vairables from configure contributing to LDFLAGS.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move LDFLAGS to mozbuild.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move symbol version script flags for arm in mozglue to moz.build.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move symbol version script ldflags for libxul to moz.build
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move symbol version script ldflags for js to moz.build
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move ldflags munging by the clang-plugin to moz.build
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Move libfuzzer ldflags filtering to moz.build.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Make a new variable for LDFLAGS to be set by Makefile.in that aren't intended to be handled by other backends.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1411712 - Fixup DEFFILE to be added to EXTRA_DEPS on windows when GNU_CC is set.
59 bytes, text/x-review-board-request
mshal
: review+
Details
No description provided.
Comment on attachment 8924211 [details] Bug 1411712 - Make a new variable for LDFLAGS to be set by Makefile.in that aren't intended to be handled by other backends. https://reviewboard.mozilla.org/r/195432/#review200576
Attachment #8924211 - Flags: review?(mshal) → review+
Comment on attachment 8923629 [details] Bug 1411712 - Make a new variable for pgo flags added to LDFLAGS. https://reviewboard.mozilla.org/r/194686/#review200578
Attachment #8923629 - Flags: review?(mshal) → review+
Comment on attachment 8923630 [details] Bug 1411712 - Use AC_SUBST_LIST instead of AC_SUBST for vairables from configure contributing to LDFLAGS. https://reviewboard.mozilla.org/r/194688/#review200580
Attachment #8923630 - Flags: review?(mshal) → review+
Comment on attachment 8923631 [details] Bug 1411712 - Move LDFLAGS to mozbuild. https://reviewboard.mozilla.org/r/194690/#review200596 ::: config/rules.mk:171 (Diff revision 2) > LINK_PDBFILE ?= $(basename $(@F)).pdb > ifdef MOZ_DEBUG > CODFILE=$(basename $(@F)).cod > endif > > ifdef DEFFILE You should be able to delete this ifdef through the endif # !GNU_CC and just replace it with: EXTRA_DEPS += $(DEFFILE) Since: 1) both the GNU_CC and !GNU_CC cases are identical and can be combined, and 2) EXTRA_DEPS += $(DEFFILE) won't do anything if DEFFILE is undefined, so there's no need to wrap it in an 'ifdef DEFFILE' anymore. ::: python/mozbuild/mozbuild/frontend/context.py:389 (Diff revision 2) > + # Used in the Windows nightlies to generate symbols for crash reporting. > + if self._context.config.substs.get('MOZ_DEBUG_SYMBOLS'): > + flags.append('-DEBUG') > + # Handle DMD in optimized builds. > + if self._context.config.substs.get('MOZ_DMD'): > + flags.append('-DEBUG') This doesn't appear to exactly match the Makefile - the MOZ_DMD case there overwrites OS_LDFLAGS to be "-DEBUG" rather than appending it. I'm not sure if that's intended, or if we should just append -DEBUG if either MOZ_DEBUG_SYMBOLS or MOZ_DMD are set. (If the latter, it might be better to append -DEBUG once so that we don't end up with it multiple times if both MOZ_DEBUG_SYMBOLS and MOZ_DMD are set). ::: python/mozbuild/mozbuild/frontend/context.py:2413 (Diff revision 2) > If tests are not enabled, this variable will be ignored. > > This variable may go away once the transition away from Makefiles is > complete. > """), > + nit: Was this whitespace added for some reason? ::: python/mozbuild/mozbuild/frontend/emitter.py:978 (Diff revision 2) > + computed_link_flags.resolve_flags('MOZBUILD', context['LDFLAGS']) > + > + deffile = context['DEFFILE'] > + if deffile and context.config.substs.get('OS_ARCH') == 'WINNT': > + if context.config.substs.get('GNU_CC'): > + computed_link_flags.resolve_flags('DEFFILE', [deffile]) The Makefile does some weird path normalization on DEFFILE - is that no longer needed in moz.build?
Attachment #8923631 - Flags: review?(mshal)
Comment on attachment 8923632 [details] Bug 1411712 - Move symbol version script flags for arm in mozglue to moz.build. https://reviewboard.mozilla.org/r/194692/#review200604 ::: mozglue/build/moz.build:101 (Diff revision 2) > # to pthread_self from tsd_init_check_recursion, which is necessary > # because somehow clang doesn't want to accept the __thread keyword > # for TLS. > LDFLAGS += ['-Wl,-bind_at_load'] > > +if CONFIG['MOZ_LINKER'] and CONFIG['CPU_ARCH'] == 'arm': Was changing TARGET_CPU to CPU_ARCH intentional? They look like they should be the same from init.configure so it's probably not an issue, though I don't know why we have separate names for cpu and raw_cpu, or if it's possible they will differ at some point. ::: mozglue/build/moz.build:102 (Diff revision 2) > # because somehow clang doesn't want to accept the __thread keyword > # for TLS. > LDFLAGS += ['-Wl,-bind_at_load'] > > +if CONFIG['MOZ_LINKER'] and CONFIG['CPU_ARCH'] == 'arm': > + if not LINK_FLAGS['OS']: Is there a way we can avoid having to do this? Can we make BaseCompileFlags a DefaultDict(list)? Or can we just do LDFLAGS += [...] instead?
Attachment #8923632 - Flags: review?(mshal)
Comment on attachment 8923633 [details] Bug 1411712 - Move symbol version script ldflags for libxul to moz.build https://reviewboard.mozilla.org/r/194694/#review200616 ::: toolkit/library/moz.build:60 (Diff revision 2) > # Eventually, the make backend would do its own path canonicalization > # and config/version.mk would lift the $(srcdir) > RCINCLUDE = '$(DEPTH)/toolkit/library/xulrunner.rc' > > + if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TARGET'] != 'Android': > + LINK_FLAGS['OS'] = CONFIG['OS_LDFLAGS'][:] + ['-Wl,-version-script,symverscript'] Can we just do LDFLAGS += ['-Wl,...'] here instead? ::: toolkit/library/moz.build:70 (Diff revision 2) > + # script however makes gold misbehave, first because it doesn't like that > + # the linker script is given after crtbegin.o, and even past that, replaces > + # the default section rules with those from the script instead of > + # supplementing them. Which leads to a lib with a huge load of sections. > + if CONFIG['OS_TARGET'] not in ('OpenBSD', 'WINNT') and CONFIG['LD_IS_BFD']: > + if 'OS' not in LINK_FLAGS or not LINK_FLAGS['OS']: Similarly here - it would be nice to avoid messing with LINK_FLAGS['OS'] and just append the StaticXULComponents.ld arg to LDFLAGS.
Attachment #8923633 - Flags: review?(mshal)
Comment on attachment 8923634 [details] Bug 1411712 - Move symbol version script ldflags for js to moz.build https://reviewboard.mozilla.org/r/194696/#review200618 ::: js/src/build/moz.build:82 (Diff revision 2) > DIST_INSTALL = True > + > +# Ensure symbol versions of shared library on Linux do not conflict > +# with those in libxul. > +if CONFIG['OS_TARGET'] == 'Linux': > + LINK_FLAGS['OS'] += ['-Wl,-version-script,symverscript'] Similar to the other patches, can we use LDFLAGS here?
Attachment #8923634 - Flags: review?(mshal)
Comment on attachment 8923635 [details] Bug 1411712 - Move ldflags munging by the clang-plugin to moz.build https://reviewboard.mozilla.org/r/194698/#review200626 ::: build/clang-plugin/moz.build:81 (Diff revision 2) > COMPILE_FLAGS['DEBUG'] = [] > COMPILE_FLAGS['OS_COMPILE_CXXFLAGS'] = [] > + > +LINK_FLAGS['OS'] = CONFIG['LLVM_LDFLAGS'] + CONFIG['CLANG_LDFLAGS'] > +# The ldflags above override most other categories. > +for var in ('DEBUG', 'WINDOWS_OPT', 'LINKER', 'OPTIMIZE'): This seems a bit dangerous - if we add a new category to context.py, it's hard to know to update this moz.build file. However, I don't have a better solution.
Attachment #8923635 - Flags: review?(mshal) → review+
Comment on attachment 8923636 [details] Bug 1411712 - Move libfuzzer ldflags filtering to moz.build. https://reviewboard.mozilla.org/r/194700/#review200628
Attachment #8923636 - Flags: review?(mshal) → review+
Comment on attachment 8923631 [details] Bug 1411712 - Move LDFLAGS to mozbuild. https://reviewboard.mozilla.org/r/194690/#review200596 > This doesn't appear to exactly match the Makefile - the MOZ_DMD case there overwrites OS_LDFLAGS to be "-DEBUG" rather than appending it. I'm not sure if that's intended, or if we should just append -DEBUG if either MOZ_DEBUG_SYMBOLS or MOZ_DMD are set. (If the latter, it might be better to append -DEBUG once so that we don't end up with it multiple times if both MOZ_DEBUG_SYMBOLS and MOZ_DMD are set). Thanks for catching that, I'm not sure what's going on here. I was able to trace the override vs. append behavior back to an ancient bug (bug 141834) that pre-dates MOZ_DMD entirely, so I don't assume this is doing the right thing, but let's keep this behavior for now and handle that in a follow up. > The Makefile does some weird path normalization on DEFFILE - is that no longer needed in moz.build? Yep, doesn't seem necessary. That code is about making sure paths have drive letters on Windows, but wherever we're setting DEFFILE it's either a bare leaf filename or starts with SRCDIR, which will have a drive letter on windows because it originates with `__file__` in python configure.
Comment on attachment 8923631 [details] Bug 1411712 - Move LDFLAGS to mozbuild. https://reviewboard.mozilla.org/r/194690/#review200596 > Thanks for catching that, I'm not sure what's going on here. I was able to trace the override vs. append behavior back to an ancient bug (bug 141834) that pre-dates MOZ_DMD entirely, so I don't assume this is doing the right thing, but let's keep this behavior for now and handle that in a follow up. Filed bug 1413728 so we can figure this out.
(In reply to Chris Manchester (:chmanchester) from comment #27) > Yep, doesn't seem necessary. That code is about making sure paths have drive > letters on Windows, but wherever we're setting DEFFILE it's either a bare > leaf filename or starts with SRCDIR, which will have a drive letter on > windows because it originates with `__file__` in python configure. Related: I have a patch in bug 1399870 to turn DEFFILE into a Path, but it apparently broke some tests. Probably wouldn't be hard to fix that and re-land it.
Comment on attachment 8923633 [details] Bug 1411712 - Move symbol version script ldflags for libxul to moz.build https://reviewboard.mozilla.org/r/194694/#review201166
Attachment #8923633 - Flags: review?(mshal) → review+
Comment on attachment 8923634 [details] Bug 1411712 - Move symbol version script ldflags for js to moz.build https://reviewboard.mozilla.org/r/194696/#review201168
Attachment #8923634 - Flags: review?(mshal) → review+
Comment on attachment 8923632 [details] Bug 1411712 - Move symbol version script flags for arm in mozglue to moz.build. https://reviewboard.mozilla.org/r/194692/#review201162
Attachment #8923632 - Flags: review?(mshal) → review+
Comment on attachment 8923631 [details] Bug 1411712 - Move LDFLAGS to mozbuild. https://reviewboard.mozilla.org/r/194690/#review201170
Attachment #8923631 - Flags: review?(mshal) → review+
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/88a1bc984357 Make a new variable for LDFLAGS to be set by Makefile.in that aren't intended to be handled by other backends. r=mshal https://hg.mozilla.org/integration/autoland/rev/aefa16acaefe Make a new variable for pgo flags added to LDFLAGS. r=mshal https://hg.mozilla.org/integration/autoland/rev/53f3dffade39 Use AC_SUBST_LIST instead of AC_SUBST for vairables from configure contributing to LDFLAGS. r=mshal https://hg.mozilla.org/integration/autoland/rev/9037f33525cb Move LDFLAGS to mozbuild. r=mshal https://hg.mozilla.org/integration/autoland/rev/60190c441710 Move symbol version script flags for arm in mozglue to moz.build. r=mshal https://hg.mozilla.org/integration/autoland/rev/e173810afc6e Move symbol version script ldflags for libxul to moz.build r=mshal https://hg.mozilla.org/integration/autoland/rev/e2910375ccd8 Move symbol version script ldflags for js to moz.build r=mshal https://hg.mozilla.org/integration/autoland/rev/3d9a54a2b527 Move ldflags munging by the clang-plugin to moz.build r=mshal https://hg.mozilla.org/integration/autoland/rev/ab9c6e310379 Move libfuzzer ldflags filtering to moz.build. r=mshal
Comment on attachment 8924735 [details] Bug 1411712 - Fixup DEFFILE to be added to EXTRA_DEPS on windows when GNU_CC is set. https://reviewboard.mozilla.org/r/195964/#review201186
Attachment #8924735 - Flags: review?(mshal) → review+
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/399dbcf3b66e Fixup DEFFILE to be added to EXTRA_DEPS on windows when GNU_CC is set. r=mshal
Depends on: 1417958
No longer depends on: 1417958
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: