New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 859989 link

Starred by 3 users
Status: Fixed
Owner:
thakis@chromium.org
Closed: Jul 13
Cc:
█ tasak@chromium.org
h...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

Clean up blink template export macros

Project Member Reported by thakis@chromium.org, Jul 3 
Blink has _TEMPLATE_CLASS_EXPORT, _EXTERN_TEMPLATE_EXPORT, _TEMPLATE_EXPORT macros.


The first one shouldn't be needed. It currently expands to nothing on windows and to the regular _EXPORT with on non-win. But how template instantiation works doesn't make sense with exports non non-explicit instantiations; we should remove it. (It sounds like the macro was added to work around the gcc warning 'type attributes ignored after type is already defined [-Werror=attributes]' -- but my guess it's a workaround there with the real fix looking different, see also the discussion on https://codereview.chromium.org/643703003/ and the progression from patch set 1 to 2 for what probably was going on.)


The 2nd and 3rd are subtly different than the version in base/export_template.h: When building a component (IMPL defined) on Windows, the blink version expands to

extern template class ___declspec(dllexport) foo<bar>;

while the base version expands to

extern template class  foo<bar>;

(in the cc file, both version then use

template class __declspec(dllexport) foo<bar>;

to actually define the template, and if IMPL isn't defined both use `extern template class __declspec(dllimport); and on non-win the behavior is identical too.)


This difference matters: cl.exe warns C4910 about having both extern and dllexport contradicting each other (we disable it by passing /wd4910 -- but only for CORE? FIXME: Why is that not needed for MODULES, PLATFORM?) and then ignores extern. clang-cl emits -Wdllexport-explicit-instantiation-decl (FIXME: ...but not in our build, only in reduced examples? Why?) and then silently ignores _dllexport_, not extern.


To do:

1. Remove _TEMPLATE_CLASS_EXPORT

2. Understand the FIXMEs above, then probably remove the blink-specific macros and use base/export_template.h (or fix the blink macros).

Comment 1 by thakis@chromium.org, Jul 3 

For the clang-cl FIXME: We map /wd4910 to -Wno-dllexport-explicit-instantiation-decl (which makes sense in a way since it's morally the same warning, but we drop the opposite thing msvc drops, so it's also misguiding in a way). The first FIXME remains.

http://llvm-cs.pcc.me.uk/tools/clang/include/clang/Driver/CLCompatOptions.td#163

Comment 2 by schenney@chromium.org, Jul 3

Components: -Blink Blink>Internals
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59aea43550238d1866c865e032633e101e7f1fa4

commit 59aea43550238d1866c865e032633e101e7f1fa4
Author: Nico Weber <thakis@chromium.org>
Date: Thu Jul 12 00:36:28 2018

Make blink's {CORE,PLATFORM,MODULES}_TEMPLATE_CLASS_EXPORT macros no-ops.

On Windows, the macro already had no effect.

Export on template classes doesn't make a lot of sense: The template itself
doesn't exist, only its instantiations do. The export is inherited by the
instantiations, so if the template had an instantiation in
core/platform/modules then that instantiation would be visible outside the
respective .so, but without an explicit instantation declaration, clients
outside those sos still get their own instantation and that doesn't really do
anything.

(The bindings generator also uses this macro on explicit template
specializations which _are_ instantiations, so the macro does have
a useful effect there -- but it looks like the effect there isn't needed
since everything builds fine with this patch applied.)

While here, also remove a warning suppression pragma for a warning that's
been disabled globally since #492411.

Bug:  859989 
Change-Id: Ib22993d06f3ad62c79e2b53b010614ae6577d227
Reviewed-on: https://chromium-review.googlesource.com/1133329
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574431}
[modify] https://crrev.com/59aea43550238d1866c865e032633e101e7f1fa4/third_party/blink/renderer/core/core_export.h
[modify] https://crrev.com/59aea43550238d1866c865e032633e101e7f1fa4/third_party/blink/renderer/modules/modules_export.h
[modify] https://crrev.com/59aea43550238d1866c865e032633e101e7f1fa4/third_party/blink/renderer/platform/platform_export.h

Comment 5 by thakis@google.com, Jul 12 

For the "FIXME: Why is that not needed for MODULES, PLATFORM?" FIXME: That's because we really use the macro correctly in core. There are uses of the macros in platform and modules, but not on explicit instantiation declarations / definitions, only on regular functions and on explicit specializations, where regular _EXPORT should be used. (Fixing in https://chromium-review.googlesource.com/c/chromium/src/+/1135300/)
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b0c866d08568364d5c3812b1f42f86552f6ab73

commit 0b0c866d08568364d5c3812b1f42f86552f6ab73
Author: Nico Weber <thakis@chromium.org>
Date: Thu Jul 12 22:41:28 2018

Remove incorrect uses of _EXTERN_TEMPLATE_EXPORT.

_EXTERN_TEMPLATE_EXPORT and _TEMPLATE_EXPORT exist for use with
explicit template instantiation declarations and definitions,
respectively.

In the bindings templates it was used on the declaration of a regular
function (whereregular _EXPORT is correct), and on the definition of a regular
function (where nothing should be used; the definition will inherit the annotation
from its declaration).

A file in platform used these macros on an explicit template specialization.
A (full) explicit specialization is a function, not a template, so use regular _EXPORT
there too.

Remove MODULES_(EXTERN_)TEMPLATE_EXPORT and PLATFORM_(EXTERN_)TEMPLATE_EXPORT,
which are unused after removing their incorrect uses. (If someone wants to
bring them back in the future, beware that they're currently slightly wrong
too, so in that case read the linked bug and see what happened to
CORE_EXTERN_TEMPLATE_EXPORT, and bring them back with the same change applied.)

NOPRESUBMIT=true

Bug:  859989 
Change-Id: I4b0218f059ff6fd71139544862a5cc3a56d0b816
Reviewed-on: https://chromium-review.googlesource.com/1135300
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574756}
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/templates/callback_function.cpp.tmpl
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/templates/callback_function.h.tmpl
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/templates/callback_interface.cpp.tmpl
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/templates/callback_interface.h.tmpl
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_test_callback_interface.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_test_callback_interface.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_test_legacy_callback_interface.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_test_legacy_callback_interface.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/modules/modules_export.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/platform/bindings/exception_messages.cc
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/platform/bindings/exception_messages.h
[modify] https://crrev.com/0b0c866d08568364d5c3812b1f42f86552f6ab73/third_party/blink/renderer/platform/platform_export.h
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47b90e1d0ce8b766c4f0f925329e828c1fe8648d

commit 47b90e1d0ce8b766c4f0f925329e828c1fe8648d
Author: Nico Weber <thakis@chromium.org>
Date: Fri Jul 13 01:34:13 2018

Remove _TEMPLATE_CLASS_EXPORT macros.

After https://chromium-review.googlesource.com/c/chromium/src/+/1133329
they are no-ops, so no behavior change.

Parts of the CL generated by running

    git grep -l CORE_TEMPLATE_CLASS_EXPORT third_party/blink/renderer/ |
        xargs sed -i 's/CORE_TEMPLATE_CLASS_EXPORT//'

Bug:  859989 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ia398e2a4aa511d10577f5ab535c7f54bc3ac47c4
Reviewed-on: https://chromium-review.googlesource.com/1135293
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574803}
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/templates/callback_function.h.tmpl
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/templates/callback_interface.h.tmpl
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_test_callback_interface.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_test_legacy_callback_interface.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/core_export.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/editing_strategy.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/ephemeral_range.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/iterators/fully_clipped_state_stack.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/iterators/simplified_backwards_text_iterator.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/iterators/text_iterator.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/position.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/position_iterator.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/position_with_affinity.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/visible_position.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/editing/visible_selection.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/layout/line/line_box_list.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/typed_arrays/dom_typed_array.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/core/typed_arrays/typed_flexible_array_buffer_view.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/modules/modules_export.h
[modify] https://crrev.com/47b90e1d0ce8b766c4f0f925329e828c1fe8648d/third_party/blink/renderer/platform/platform_export.h
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85f40d183c0a19b6acbe5fb89e59aae068c533b4

commit 85f40d183c0a19b6acbe5fb89e59aae068c533b4
Author: Nico Weber <thakis@chromium.org>
Date: Fri Jul 13 15:05:19 2018

win: Make CORE_EXTERN_TEMPLATE_EXPORT expand to nothing while building core.

Without this, when building core,
`extern template class CORE_EXTERN_TEMPLATE_EXPORT foo<bar>;`
expands to
`extern template class __declspec(dllexport) foo<bar>;`
in a .h file using CORE_EXTERN_TEMPLATE_EXPORT.

The "extern" means "this template foo is explicitly instantiated with type parameter
bar in a cc file somewhere, so don't bother generating code for it if someone
uses it", while "dllexport" means "we're exporting this template foo with
type parameter bar from the dll we're currently building, so please make sure
to generate code for it". These two contradict each other.

cl emits C4910 and ignores the "extern" -- meaning the code for foo<bar> is
emitted into every translation unit in core including this header.  This
is harmless, but if we were still build with msvc it'd be slower than it needs
to be. (Then again, MSVC says "The extern keyword in the specialization only applies
to member functions defined outside of the body of the class. Functions defined inside
the class declaration are considered inline functions and are always instantiated.",
so maybe it doesn't really have an effect there either since all our templates
have all their functions inline, more or less by necessity.)

clang-cl emits Wdllexport-explicit-instantiation-decl (suppressed by /wd4910)
and ignores the dllexport. There, this patch doesn't change behavior but
the warning is no longer emitted.

(In either case, the cc file then says CORE_TEMPLATE_EXPORT on the instantiation
definition which _does_ export to __declspec(dllexport) and makes sure code for
foo<bar> is generated -- but only in a single cc file.)

So this patch doesn't really change behavior, but it allows us to no longer pass
/wd4910. It also makes CORE_EXTERN_TEMPLATE_EXPORT's behavior match
base/export_template.h and is a step towards maybe using that directly.

See bug comment 0 for more details.

Also remove a few CORE_EXPORTs on template classes. See the CL description of
ttps://chromium-review.googlesource.com/1133329 for why they make no sense,
and if they're there then the EXPORT gets inherited from the template class
to the specialization, triggering the warning again.

Bug:  859989 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I1832bdf3d618a72be9b4d78cb435303ca1517677
Reviewed-on: https://chromium-review.googlesource.com/1135937
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574919}
[modify] https://crrev.com/85f40d183c0a19b6acbe5fb89e59aae068c533b4/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/85f40d183c0a19b6acbe5fb89e59aae068c533b4/third_party/blink/renderer/core/core_export.h
[modify] https://crrev.com/85f40d183c0a19b6acbe5fb89e59aae068c533b4/third_party/blink/renderer/core/editing/iterators/character_iterator.h
[modify] https://crrev.com/85f40d183c0a19b6acbe5fb89e59aae068c533b4/third_party/blink/renderer/core/editing/selection_template.h
[modify] https://crrev.com/85f40d183c0a19b6acbe5fb89e59aae068c533b4/third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h

Comment 9 by thakis@chromium.org, Jul 13

Status: Fixed (was: Assigned)
We could still make core use base/template_export.h, but I've done what I wanted to do here.

Comment 10 by raphael....@intel.com, Jul 19 

https://chromium-review.googlesource.com/1133329 breaks linking with GCC 7 and 8 and my system's binutils (2.29):

obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_CollapseAroundRep
lacedElement_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::Append(WTF::String const&, blink::ComputedStyle const*, blink::LayoutText*)'                                                                                                                                                                               
obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_CollapseAroundReplacedElement_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::AppendAtomicInline(blink::ComputedStyle const*, blink::LayoutObject*)'
obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_CollapseAroundReplacedElement_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::Append(WTF::String const&, blink::ComputedStyle const*, blink::LayoutText*)'
obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_CollapseAroundRep
lacedElement_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::~NGInlineItemsBuilderTemplate()'                            
obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_BidiBlockOverride
_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::EnterBlock(blink::ComputedStyle const*)'                                
obj/third_party/blink/renderer/core/unit_tests/ng_inline_items_builder_test.o:ng_inline_items_builder_test.cc:function blink::(anonymous namespace)::NGInlineItemsBuilderTest_BidiBlockOverride
_Test::TestBody(): error: undefined reference to 'blink::NGInlineItemsBuilderTemplate<blink::NGOffsetMappingBuilder>::Append(WTF::String const&, blink::ComputedStyle const*, blink::LayoutText
*)'

[and lots of others all in the same .o]

Adding CORE_EXPORT to NGInlineItemsBuilderTemplate fixes it, but NGInlineItemsBuilderTemplate is a template class, so I'm wondering if there's something else that needs to be done here.

Comment 11 by raphael....@intel.com, Jul 24 

ping?

Comment 12 by thakis@chromium.org, Jul 24 

Sorry, I wrote a reply in a tab (I even found that tab!) but forgot to hit "Save changes". Here it comes:

NGInlineItemsBuilderTemplate is a bit weird because it has this specialization and this explicit instantiation declaration: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h?type=cs&q=NGInlineItemsBuilderTemplate&sq=package:chromium&g=0&l=159

Both the specialization and the instantiation have export macros, so I wonder why that's not working with gcc. Can you try to make a reduced repro showing the difference?

I don't understand what model gcc uses here internally.

Comment 13 by raphael....@intel.com, Oct 18 

I didn't have time to investigate this further, but for the record I've just built the "blink_tests" and "unittests" targets with GCC 7 and ld.bfg from binutils 2.29 and everything worked.
►

Sign in to add a comment