Closed Bug 1382857 Opened 7 years ago Closed 7 years ago

Undefined symbols for architecture x86_64 "vtable for FontInfoData"

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bradwerth, Assigned: bradwerth)

Details

Attachments

(1 file, 1 obsolete file)

Work around clang bug PR34163
811 bytes, patch
froydnj
: review+
Details | Diff | Splinter Review 
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.
Attachment #8888552 - Flags: review?(milan)
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...)
(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 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)?
Attachment #8888552 - Flags: review?(milan) → review?(jfkthame)
(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.
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
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)?
(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.
(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?
(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
(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?
(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.
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.
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
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
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;
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() {
}
This has been fixed on clang trunk. It hasn't been merged to 5.0 branch yet.
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.
Attachment #8888552 - Attachment is obsolete: true
Attachment #8888552 - Flags: review?(jfkthame)
Attachment #8896600 - Flags: review?(nfroyd)
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 #?
Attachment #8896600 - Flags: review?(nfroyd) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: