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 / TestsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, ews, Hironori.Fujii, lforschler, mcatanzaro, mmaxfield, webkit-bug-importer
Priority: P2 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

Description Frédéric Wang (:fredw) 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.
Comment 1 Frédéric Wang (:fredw) 2018-09-12 03:09:22 PDT
Created attachment 349537 [details]
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag)
Comment 2 Frédéric Wang (:fredw) 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.
Comment 3 Frédéric Wang (:fredw) 2018-09-12 11:37:37 PDT
Created attachment 349566 [details]
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag)
Comment 4 Alex Christensen 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?
Comment 5 Frédéric Wang (:fredw) 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
Comment 6 Frédéric Wang (:fredw) 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).
Comment 7 Frédéric Wang (:fredw) 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.
Comment 8 Fujii Hironori 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.
Comment 9 Alex Christensen 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.
Comment 10 Frédéric Wang (:fredw) 2018-09-14 02:38:30 PDT
Created attachment 349755 [details]
Patch (move template functions to header)
Comment 11 Frédéric Wang (:fredw) 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
Comment 12 Frédéric Wang (:fredw) 2018-09-14 07:36:07 PDT
Created attachment 349758 [details]
Patch (declare fully specialized functions in FontTaggedSetting.h)
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2018-09-14 11:33:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-09-14 11:34:32 PDT
<rdar://problem/44463595>