Bug 189541

Summary:

Build error in FontDescriptionKey::computeHash when compiling FontTaggedSettings and FontCascadeFonts together

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

Tools / Tests

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, commit-queue, ews-watchlist, fujii.hironori, lforschler, mcatanzaro, mmaxfield, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

188043

Attachments:

Description	Flags
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag)	none
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag)	none
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file)	none
Patch (move template functions to header)	none
Patch (declare fully specialized functions in FontTaggedSetting.h)	none

Frédéric Wang (:fredw)

Reported 2018-09-12 02:50:08 PDT

Applying attachment 349536 [details], I get the errors below on macOS and iOS ports. Since attachment 349536 [details] is unrelated to font stuff I suspect this is one more instance of UnifiedBuild rotating. A workaround is to move the implementation from the header to the cpp file. In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:8: ./platform/graphics/FontTaggedSettings.cpp:36:31: error: explicit specialization of 'hash' after instantiation unsigned FontFeatureSettings::hash() const ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:1: In file included from ./platform/graphics/FontCascadeFonts.cpp:32: ./platform/graphics/FontCache.h:117:38: note: implicit instantiation first required here hasher.add(m_featureSettings.hash()); ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:8: ./platform/graphics/FontTaggedSettings.cpp:48:33: error: explicit specialization of 'hash' after instantiation unsigned FontVariationSettings::hash() const ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:1: In file included from ./platform/graphics/FontCascadeFonts.cpp:32: ./platform/graphics/FontCache.h:119:40: note: implicit instantiation first required here hasher.add(m_variationSettings.hash()); ^ 2 errors generated.

Attachments
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag) (3.67 KB, patch) 2018-09-12 03:09 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag) (3.48 KB, patch) 2018-09-12 11:37 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file) (3.43 KB, patch) 2018-09-13 03:58 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (move template functions to header) (2.71 KB, patch) 2018-09-14 02:38 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (declare fully specialized functions in FontTaggedSetting.h) (2.51 KB, patch) 2018-09-14 07:36 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2018-09-12 03:09:22 PDT

Created attachment 349537 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag)

Frédéric Wang (:fredw)

Comment 2 2018-09-12 11:32:30 PDT

(In reply to Myles C. Maxfield from comment #15) > Comment on attachment 349552 [details] > Patch > > We often inline functions for performance, and fonts are used pretty often. > Is this a performance regression? ALWAYS_INLINE failed to build.

Frédéric Wang (:fredw)

Comment 3 2018-09-12 11:37:37 PDT

Created attachment 349566 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag)

Alex Christensen

Comment 4 2018-09-12 13:21:01 PDT

Comment on attachment 349566 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag) View in context: https://bugs.webkit.org/attachment.cgi?id=349566&action=review > Source/WebCore/platform/graphics/FontCache.h:107 > + inline unsigned computeHash() const; This "inline" doesn't do anything any more. Is computeHash only used from within FontCache.cpp?

Frédéric Wang (:fredw)

Comment 5 2018-09-12 21:39:15 PDT

For the record, my initial attempt was in https://bugs.webkit.org/show_bug.cgi?id=185087#c14

Frédéric Wang (:fredw)

Comment 6 2018-09-13 03:05:01 PDT

Just to complete the description of the bug, the unified build content is: #include "platform/graphics/FontCascadeFonts.cpp" #include "platform/graphics/FontDescription.cpp" #include "platform/graphics/FontFamilySpecificationNull.cpp" #include "platform/graphics/FontGenericFamilies.cpp" #include "platform/graphics/FontPlatformData.cpp" #include "platform/graphics/FontRanges.cpp" #include "platform/graphics/FontSelectionAlgorithm.cpp" #include "platform/graphics/FontTaggedSettings.cpp" So the implicit instantiation happens in FontCache.h (included by FontCascadeFonts.cpp) before the explicit specialization (in FontTaggedSettings.cpp).

Frédéric Wang (:fredw)

Comment 7 2018-09-13 03:58:45 PDT

Created attachment 349651 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file) This is attachment 349552 [details] (attached to the wrong bug) which did pass EWS but does not do any inlining.

Fujii Hironori

Comment 8 2018-09-13 07:07:22 PDT

A template function should be defined in header, not in a cpp file. FontFeatureSettings::hash() should be defined in FontTaggedSettings class definition.

Alex Christensen

Comment 9 2018-09-13 09:09:16 PDT

(In reply to Fujii Hironori from comment #8) > A template function should be defined in header, not in a cpp file. > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > definition. You're absolutely right. We should move FontFeatureSettings::hash and FontVariationSettings::hash to a header.

Frédéric Wang (:fredw)

Comment 10 2018-09-14 02:38:30 PDT

Created attachment 349755 [details] Patch (move template functions to header)

Frédéric Wang (:fredw)

Comment 11 2018-09-14 06:45:26 PDT

(In reply to Fujii Hironori from comment #8) > A template function should be defined in header, not in a cpp file. > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > definition. (In reply to Alex Christensen from comment #9) > (In reply to Fujii Hironori from comment #8) > > A template function should be defined in header, not in a cpp file. > > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > > definition. > > You're absolutely right. We should move FontFeatureSettings::hash and > FontVariationSettings::hash to a header. Thanks for the suggestion. I tried that in attachment 349755 [details] but as you can see linking fails with duplicate symbols. I can probably avoid this by setting the functions as inline but anyway I realize this is not relevant here. These two functions are already fully specialized so they are not template functions! The issue in comment 0 is "Explicit specialization must be declared before the first use that would cause implicit instantiation, in every translation unit where such use occurs" [1]. The problem is that unified build may put FontCache.h and FontTaggedSettings.cpp in the same unit. This can be avoided by moving FontDescriptionKey::computeHash to FontCache.cpp as done in attachment 349651 [details] but it could really still be possible that unified build puts FontCache.cpp and FontTaggedSettings.cpp in the same unit... So my guess is that the proper solution is to isolate FontTaggedSettings.cpp as suggested by Michael on webkit-dev. [1] https://en.cppreference.com/w/cpp/language/template_specialization

Frédéric Wang (:fredw)

Comment 12 2018-09-14 07:36:07 PDT

Created attachment 349758 [details] Patch (declare fully specialized functions in FontTaggedSetting.h)

WebKit Commit Bot

Comment 13 2018-09-14 11:33:53 PDT

Comment on attachment 349758 [details] Patch (declare fully specialized functions in FontTaggedSetting.h) Clearing flags on attachment: 349758 Committed r236013: <https://trac.webkit.org/changeset/236013>

WebKit Commit Bot

Comment 14 2018-09-14 11:33:55 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 15 2018-09-14 11:34:32 PDT

<rdar://problem/44463595>

Note You need to log in before you can comment on or make changes to this bug.