Closed Bug 1411712 Opened 7 years ago Closed 7 years ago

Move ldflags to moz.build

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 1 open bug)

Details

Attachments

(10 files)

59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
59 bytes, text/x-review-board-request
mshal
: review+
Details
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.