1469091 - --enable-lto is not compatible with --enable-clang-plugin

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

When going past the restrictions described in bug 1469088, the build fails with:

[task 2018-06-16T00:14:30.314Z] 00:14:30     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -std=gnu++14 -Qunused-arguments -flto=thin -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I/builds/worker/workspace/build/src/clang/include -fPIC -static-libstdc++ -fvisibility-inlines-hidden -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wnon-virtual-dtor -fcolor-diagnostics -O3 -DNDEBUG -fno-exceptions -fno-rtti -DLLVM_BUILD_GLOBAL_ISEL -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DHAVE_NEW_ASTMATCHER_NAMES -DHAS_ACCEPTS_IGNORINGPARENIMPCASTS -fno-rtti -fno-exceptions -fno-omit-frame-pointer -Werror  -fPIC -shared -o libclang-plugin.dylib ThirdPartyPaths.o Unified_cpp_build_clang-plugin0.o Unified_cpp_build_clang-plugin1.o  -L/builds/worker/workspace/build/src/clang/lib -lclangASTMatchers      -dynamiclib -install_name @executable_path/libclang-plugin.dylib -compatibility_version 1 -current_version 1 -single_module
[task 2018-06-16T00:14:30.314Z] 00:14:30     INFO -  /usr/bin/ld: /builds/worker/workspace/build/src/clang/bin/../lib/LLVMgold.so: error in plugin cleanup (ignored)
[task 2018-06-16T00:14:30.315Z] 00:14:30     INFO -  /usr/bin/ld: /builds/worker/workspace/build/src/clang/bin/../lib/LLVMgold.so: error loading plugin
[task 2018-06-16T00:14:30.315Z] 00:14:30     INFO -  clang-5.0: [0;1;31merror: [0mlinker command failed with exit code 1 (use -v to see invocation)[0m
[task 2018-06-16T00:14:30.315Z] 00:14:30     INFO -  /builds/worker/workspace/build/src/config/rules.mk:679: recipe for target 'libclang-plugin.dylib' failed
[task 2018-06-16T00:14:30.315Z] 00:14:30     INFO -  make[4]: *** [libclang-plugin.dylib] Error 1

That's because clang is compiling the clang-plugin sources with -flto, but the linker can't deal with that. The core problem is that we don't have actual host shared libraries, the short term fix might be to strip out the MOZ_LTO_*FLAGS when linking it.

Comment 1

[(not currently active) Ted Mielczarek]

I feel like I keep saying it, but maybe *this* time we should just make host shared libraries work?

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1469091 - Build the clang plugin as a host shared library.

This adds just enough host shared library support for this one use case,
but also takes shortcuts, because fully supporting host shared library
is a deep rabbit hole I'm not ready to take just to fix --enable-lto
--enable-clang-plugin on mac builds.

One downside is that one my machine the plugin now takes > 80s to build,
instead of 15s before, thanks to the lack of unified sources.

Review commit: https://reviewboard.mozilla.org/r/255156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255156/

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8990151 [details]
Bug 1469091 - Build the clang plugin as a host shared library.

https://reviewboard.mozilla.org/r/255156/#review262178

Thanks, this is definitely better than the status quo!

::: build/clang-plugin/Makefile.in:7
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  include $(topsrcdir)/config/config.mk
>  
> -# In the current moz.build world, we need to override essentially every
> +HOST_LDFLAGS := $(LLVM_LDFLAGS) $(CLANG_LDFLAGS)

Can these get moved to moz.build now?

::: build/clang-plugin/moz.build:11
(Diff revision 1)
>  
> -SharedLibrary('clang-plugin')
> +HostSharedLibrary('clang-plugin')
>  
> -SOURCES += ['!ThirdPartyPaths.cpp']
> +HOST_SOURCES += ['!ThirdPartyPaths.cpp']
>  
> -UNIFIED_SOURCES += [
> +HOST_SOURCES += [

File a followup on `HOST_UNIFIED_SOURCES`? Or maybe we could just try making all `HOST_SOURCES` build as unified and see if anything breaks? There aren't that many of them.

::: build/templates.mozbuild:110
(Diff revision 1)
>  
>  @template
> +def HostSharedLibrary(name):
> +    '''Template for build tools libraries.'''
> +    if name != 'clang-plugin':
> +        error('Please make sure host shared library support is complete '

Good thinking! This will assuredly save someone headache in the future.

::: config/rules.mk:678
(Diff revision 1)
>  
> +$(HOST_SHARED_LIBRARY): $(HOST_OBJS) Makefile
> +	$(REPORT_BUILD)
> +	$(RM) $@
> +ifdef _MSC_VER
> +	# /!\ We assume host and target are using the same compiler

Considering that you only implemented enough of this for the clang-plugin, maybe it's not worthwhile to even have this MSVC block?

::: python/mozbuild/mozbuild/backend/recursivemake.py:1286
(Diff revision 1)
>  
>      def _process_host_library(self, libdef, backend_file):
>          backend_file.write('HOST_LIBRARY_NAME = %s\n' % libdef.basename)
>  
> +    def _process_host_shared_library(self, libdef, backend_file):
> +        backend_file.write('HOST_SHARED_LIBRARY = %s\n' % libdef.lib_name)

nit: this could be :=

::: python/mozbuild/mozbuild/frontend/data.py:770
(Diff revision 1)
>  
> +class HostSharedLibrary(HostMixin, Library):
> +    """Context derived container object for a host shared library.
> +
> +    This class supports less things than SharedLibrary does for target shared
> +    libraries."""

Please put a note in the docstring about the fact that this is limited to supporting the clang-plugin currently.

::: python/mozbuild/mozbuild/frontend/emitter.py:357
(Diff revision 1)
>          for lib in context.get(variable.replace('USE', 'OS'), []):
>              obj.link_system_library(lib)
>  
>          # We have to wait for all the self._link_library calls above to have
>          # happened for obj.cxx_link to be final.
> -        if not isinstance(obj, (StaticLibrary, HostLibrary,
> +        # FIXME: Theoretically, HostSharedLibrary shouldn't be here.

File a bug for this FIXME and put the bug number here, please.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8990151 [details]
Bug 1469091 - Build the clang plugin as a host shared library.

https://reviewboard.mozilla.org/r/255156/#review262178

> Can these get moved to moz.build now?

We can't *replace* HOST_LDFLAGS in moz.build land.

> File a followup on `HOST_UNIFIED_SOURCES`? Or maybe we could just try making all `HOST_SOURCES` build as unified and see if anything breaks? There aren't that many of them.

Or maybe, as suggested in bug 1409276, get rid of the separate notion for host things.

> Considering that you only implemented enough of this for the clang-plugin, maybe it's not worthwhile to even have this MSVC block?

We have static analysis jobs on windows, using the plugin.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8990151 [details]
Bug 1469091 - Build the clang plugin as a host shared library.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255156/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/531b966781e6
Build the clang plugin as a host shared library. r=ted

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/531b966781e6