1382857 - Undefined symbols for architecture x86_64 "vtable for FontInfoData"

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Brad Werth [:bradwerth] | Out until Feb 2025]

macOS clang compilation fails with:

Undefined symbols for architecture x86_64:
"vtable for FontInfoData", referenced from:
  FontInfoData::FontInfoData(bool, bool, bool) in Unified_mm_gfx_thebes0.o
  FontInfoData::~FontInfoData() in Unified_mm_gfx_thebes0.o
NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.

And it appears that this explanation is, in fact, the case.

Comment 1

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1382857 Part 1: Move FontInfoData constructor out of inline, creating a vtable needed for clang compilation.

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

Comment 2

[Jonathan Kew [:jfkthame]]

I don't understand this... is it really some kind of unified-compilation-related bustage? (And why isn't the build failing for other people? I build successfully with clang on macOS, and I'm sure I'm not alone...)

Comment 3

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> I don't understand this... is it really some kind of
> unified-compilation-related bustage? (And why isn't the build failing for
> other people? I build successfully with clang on macOS, and I'm sure I'm not
> alone...)

It could be my toolchain. I'm using a macports mp-clang-devel install, version 5.0.0. Perhaps this is a vision of the future for other macOS developers.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8888552 [details]
Bug 1382857 Part 1: Move FontInfoData constructor out of inline, creating a vtable needed for clang compilation.

https://reviewboard.mozilla.org/r/159526/#review165178

I can't even begin to understand why this would fix things :), but I want to make sure we are not messing with something that is performance sensitive.  Jonathan, is this safe as a performance issue (making the FontInfoData constructor non-inline)?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Brad Werth [:bradwerth] from comment #3)
>...
> 
> It could be my toolchain. I'm using a macports mp-clang-devel install,
> version 5.0.0. Perhaps this is a vision of the future for other macOS
> developers.

Let's understand the problem better before making a change like this.  Or at least reproduce on multiple machines.

Comment 6

[Brad Werth [:bradwerth] | Out until Feb 2025]

StackOverflow has a discussion of why clang flags this issue: https://stackoverflow.com/a/40597174/3325423

My build environment:

$ clang --version
clang version 5.0.0 (trunk 306254)
Target: x86_64-apple-darwin16.7.0
Thread model: posix

Comment 7

[Jonathan Kew [:jfkthame]]

Maybe I'm not understanding adequately, but AFAICS that SO question is somewhat different; it's about a clang warning "no out-of-line virtual method definitions", which is not the issue here.

Still, taking a guess... does it by any chance resolve the issue for you if you de-inline the override of Load() in the MacFontInfo subclass (in gfxMacPlatformFontList.mm)?

Comment 8

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Maybe I'm not understanding adequately, but AFAICS that SO question is
> somewhat different; it's about a clang warning "no out-of-line virtual
> method definitions", which is not the issue here.

Yes, but the answer I linked to explains the issue I am encountering. The answer links back to the Itanium C++ ABI https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable which summarizes like so:

> The virtual table for a class is emitted in the same object containing the definition of its key function, i.e. the first non-pure virtual function that is not inline at the point of class definition. If there is no key function, it is emitted everywhere used.

If this is an accurate description of what's happening -- and I understand that's what we're sorting out -- it would seem that my proposed patch would avoid duplicating the vtable. I don't know why that's showing up in my clang compilation and not in others. But it may be silently happening in Firefox builds elsewhere, given that the structure of the FontInfoData class is a clear example of what this Itanium C++ ABI note mentions.

> Still, taking a guess... does it by any chance resolve the issue for you if
> you de-inline the override of Load() in the MacFontInfo subclass (in
> gfxMacPlatformFontList.mm)?

No; this has no effect.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Brad Werth [:bradwerth] from comment #8)
> (In reply to Jonathan Kew (:jfkthame) from comment #7)
> > Maybe I'm not understanding adequately, but AFAICS that SO question is
> > somewhat different; it's about a clang warning "no out-of-line virtual
> > method definitions", which is not the issue here.
> 
> Yes, but the answer I linked to explains the issue I am encountering. The
> answer links back to the Itanium C++ ABI
> https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable which
> summarizes like so:
> 
> > The virtual table for a class is emitted in the same object containing the definition of its key function, i.e. the first non-pure virtual function that is not inline at the point of class definition. If there is no key function, it is emitted everywhere used.
> 
> If this is an accurate description of what's happening -- and I understand
> that's what we're sorting out -- it would seem that my proposed patch would
> avoid duplicating the vtable. I don't know why that's showing up in my clang
> compilation and not in others. But it may be silently happening in Firefox
> builds elsewhere, given that the structure of the FontInfoData class is a
> clear example of what this Itanium C++ ABI note mentions.

According to the C++ ABI doc, the key function would be FontInfoData::Load (it is a virtual function, not pure, and not inline at the class definition)...and that is already defined in gfxFontLoader.cpp.  So why is clang deciding to not output the vtable in that translation unit?

Comment 10

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Nathan Froyd [:froydnj] from comment #9)

> According to the C++ ABI doc, the key function would be FontInfoData::Load
> (it is a virtual function, not pure, and not inline at the class
> definition)...and that is already defined in gfxFontLoader.cpp.  So why is
> clang deciding to not output the vtable in that translation unit?

For reference, the relevant clang source is http://clang.llvm.org/doxygen/RecordLayoutBuilder_8cpp_source.html#l01989

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Brad Werth [:bradwerth] from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> 
> > According to the C++ ABI doc, the key function would be FontInfoData::Load
> > (it is a virtual function, not pure, and not inline at the class
> > definition)...and that is already defined in gfxFontLoader.cpp.  So why is
> > clang deciding to not output the vtable in that translation unit?
> 
> For reference, the relevant clang source is
> http://clang.llvm.org/doxygen/RecordLayoutBuilder_8cpp_source.html#l01989

That code is super-stable, though; the most recent change to it was 2 years ago:

https://github.com/llvm-mirror/clang/blame/master/lib/AST/RecordLayoutBuilder.cpp

What does |grep '#include' Unified_mm_gfx_thebes0.cpp| say?  Or, relatedly, what unified file is gfxFontLoader.cpp included in?

Does it make a difference to put gfxFontLoader.cpp in SOURCES rather than UNIFIED_SOURCES?

Comment 12

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> What does |grep '#include' Unified_mm_gfx_thebes0.cpp| say?  Or, relatedly,
> what unified file is gfxFontLoader.cpp included in?

Unified_cpp_gfx_thebes0.cpp does include gfxFontInfoLoader.cpp. It includes, in order:

CJKCompatSVS.cpp
SoftwareVsyncSource.cpp
VsyncSource.cpp
gfxAlphaRecovery.cpp
gfxBaseSharedMemorySurface.cpp
gfxBlur.cpp
gfxContext.cpp
gfxFont.cpp
gfxFontEntry.cpp
gfxFontFeatures.cpp
gfxFontInfoLoader.cpp
gfxFontMissingGlyphs.cpp
gfxFontSrcPrincipal.cpp
gfxFontSrcURI.cpp
gfxGlyphExtents.cpp
gfxGradientCache.cpp

> Does it make a difference to put gfxFontLoader.cpp in SOURCES rather than
> UNIFIED_SOURCES?

In that case I get a compilation error:

gfxFontInfoLoader.cpp:33:13: error: use of undeclared identifier 'gfxCriticalError'

I'll see if I can get through that and test your idea more thoroughly.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

I was able to reduce this down to the following scenario:

#include "gfxFontInfoLoader.cpp"

compiles fine once gfxCriticalError issue is fixed and emits a vtable for FontInfoData

#include "gfxFont.cpp"
#include "gfxFontInfoLoader.cpp"

Compiling gfxFont and gfxFontInfoLoader together causes the vtable to stop being emitted.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Reducing the problem further: Appending the following to gfxFontInfoLoader.cpp causes the the vtable to stop being emitted.

#include "gfxFontEntry.h"
class gfxUserFontSet {
  gfxFontFamily *Get();
};

class nsIDocument {
    gfxUserFontSet* GetUserFontSet(bool aFlushUserFontSet = true);
};
namespace mozilla {
  class ServoElementSnapshot;
  namespace dom {
    class StyleChildrenIterator;
  } // namespace dom
} // namespace mozilla

using mozilla::dom::StyleChildrenIterator;
using mozilla::ServoElementSnapshot;
class nsIDocument;
class jrmNOd {
  public:
  nsIDocument* mDocument;
};
class nsAttrName
{
public:
  explicit nsAttrName(jrmNOd* aNodeInfo) { }
};
namespace mozilla {

struct ServoAttrSnapshot
{
  nsAttrName mName;
};

class ServoElementSnapshot
{
private:
  nsTArray<ServoAttrSnapshot> mAttrs;
};

} // namespace mozilla

Comment 15

[Jeff Muizelaar [:jrmuizel]]

And here's a version with no additional includes. It looks like FontInfoData is being forward declared and then there's a long chain of references to the type and that confuses clang somehow? I'll try to reduce further.

class FontInfoData;
class gfxFontFamily2 {
      virtual void ReadCMAP(FontInfoData *aFontInfoData = nullptr);
};
class gfxUserFontSet {
  gfxFontFamily2 *Get();
};

class nsIDocument {
    gfxUserFontSet* GetUserFontSet(bool aFlushUserFontSet = true);
};
namespace mozilla {
  class ServoElementSnapshot;
  namespace dom {
    class StyleChildrenIterator;
  } // namespace dom
} // namespace mozilla

using mozilla::dom::StyleChildrenIterator;
using mozilla::ServoElementSnapshot;
class nsIDocument;
class jrmNOd {
  public:
  nsIDocument* mDocument;
};
class nsAttrName
{
public:
  explicit nsAttrName(jrmNOd* aNodeInfo) { }
};
namespace mozilla {
struct ServoAttrSnapshot
{
  nsAttrName mName;
};

class ServoElementSnapshot
{
private:
  ServoAttrSnapshot mAttrs;
};
} // namespace mozilla

Comment 16

[Jeff Muizelaar [:jrmuizel]]

And here's a further reduced prefix:

class FontInfoData;

class A {
  void F(FontInfoData *aFontInfoData);
};

namespace mozilla {
class B
{
public:
  A* mAttrs;
};

}

using mozilla::B;

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Here's a completely standalone reproduction with clang version 5.0.0. It's necessary to compile with '-g' or it doesn't reproduce. Removing the 'using mozilla::B' line causes the vtable to be emitted otherwise it's not. This seems like a compiler bug. 

class FontInfoData;

class A {
          void F(FontInfoData *aFontInfoData);
};

namespace mozilla {
        struct B
        {
                A* mAttrs;
        };
}

using mozilla::B;

using namespace mozilla;
class FontInfoData {
        void Release() {
                delete this;
        }
        virtual ~FontInfoData() {}
        virtual void R();
};

void FontInfoData::R() {
}

Comment 18

[Jeff Muizelaar [:jrmuizel]]

I filled: https://bugs.llvm.org/show_bug.cgi?id=34163

Comment 19

[Jeff Muizelaar [:jrmuizel]]

This has been fixed on clang trunk. It hasn't been merged to 5.0 branch yet.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Attachment: Work around clang bug PR34163

This is the suggested workaround by the clang developers. The issue has been fixed on clang trunk but hasn't made its way to 5.0 yet. It's probably best to work around it for now just so that no one has to waste time. We can probably remove it after the upstream fix has had time to propagate.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8896600 [details] [diff] [review]
Work around clang bug PR34163

Review of attachment 8896600 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontInfoLoader.h
@@ +61,5 @@
>      }
>  
>  protected:
>      // Protected destructor, to discourage deletion outside of Release():
> +    inline virtual ~FontInfoData() {

Inline virtuals are pretty unusual...maybe a short comment here saying the inline is to work around a clang bug, with the clang PR #?

Comment 22

[Brad Werth [:bradwerth] | Out until Feb 2025]

The fix to clang has worked its way into MacPorts clang-devel "clang version 6.0.0 (trunk 311629)", so I no longer need a change to the Firefox code to compile successfully. Marking as WONTFIX.