[C++20] [Modules] Clang didn't generate the virtual table to the unit containing the key function

[ChuanqiXu9]

Reproducer:

// layer1.cppm
export module foo:layer1;
struct Fruit {
    virtual ~Fruit() = default;
    virtual void eval() = 0;
};
struct Banana : public Fruit {
    Banana() {}
    void eval() override;
};

// layer2.cppm
export module foo:layer2;
import :layer1;
export void layer2_fun() {
    Banana *b = new Banana();
    b->eval();
}
void Banana::eval() {
}

Compiler layer2.cppm with:

clang++ -std=c++20 layer1.cppm --precompile -o foo-layer1.pcm
clang++ -std=c++20 layer2.cppm -fprebuilt-module-path=. -S -emit-llvm -o -

Then we can't see anything about the definition of virtual table. This violates the itanium ABI 5.2.3:

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.

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Reproducer:

// layer1.cppm
export module foo:layer1;
struct Fruit {
    virtual ~Fruit() = default;
    virtual void eval() = 0;
};
struct Banana : public Fruit {
    Banana() {}
    void eval() override;
};

// layer2.cppm
export module foo:layer2;
import :layer1;
export void layer2_fun() {
    Banana *b = new Banana();
    b->eval();
}
void Banana::eval() {
}

Compiler layer2.cppm with:

clang++ -std=c++20 layer1.cppm --precompile -o foo-layer1.pcm
clang++ -std=c++20 layer2.cppm -fprebuilt-module-path=. -S -emit-llvm -o -

Then we can't see anything about the definition of virtual table. This violates the itanium ABI 5.2.3:

> 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.

[iains]

I seem to recall we fixed a key function issue not too long ago - is this problem present on earlier branches?

[ChuanqiXu9]

I seem to recall we fixed a key function issue not too long ago - is this problem present on earlier branches?

No, I tested on trunk.

The problem we fixed were about we generate the vtables in too many units to produce duplicated symbols.

But the problem is about we didn't generate the vtable in the targeted unit so that we met undefined symbol.

I am trying to look at it. Probably an oversight.

[ChuanqiXu9]

I took a simple look at the problem and the cause may be a mismatch.

Currently, (maybe there are other paths), when we're generating the constructor of the record decl, we will try to emit the vtable for the record decl if the record decl is a dynamic class.

However, in this example, the constructor won't be generated when we compile layer2.cppm, then here is the problem.

One solution may be, checking if the current emitting function is a key function and if yes, try to generate the vtable.

[rnikander]

FYI, when I create a similar thing in the old style, with just .cc and .o files, I see things like this:

% nm layer1_b.o | llvm-cxxfilt | grep vtable 
00000000000000a0 S vtable for Fruit
                 U vtable for Banana
                 U vtable for __cxxabiv1::__class_type_info
% nm layer2_b.o | llvm-cxxfilt | grep vtable
0000000000000d68 S vtable for Banana
                 U vtable for __cxxabiv1::__class_type_info
                 U vtable for __cxxabiv1::__si_class_type_info

It's seems it's emitting the vtables in one .o files and resolving undefined U vtable for Banana later during linking.

[davidstone]

Is the concept of a key function necessary with modules? Would anything bad happen if the virtual table were emitted in the object file for the module that defines the class?

[ChuanqiXu9]

Is the concept of a key function necessary with modules? Would anything bad happen if the virtual table were emitted in the object file for the module that defines the class?

Otherwise we're violating the Itanium ABI, which seems not good.

[dwblaikie]

But modules are a new space for the ABI - we could change/specify the ABI to home any type defined in a module to that modules object file, right? I think that would be a good ABI feature/change/direction for modules - at least at a glance. Perhaps on discussion there might be complications/issues with doing that.

[dwblaikie]

(I'd want to do/have done similar things with debug info for Clang Header Modules, -fmodules-debuginfo that homes DWARF type definitions to the modular object file - this makes even more sense in a C++ standard modules world where object files are required to be generated from modules)

[ChuanqiXu9]

I filed itanium-cxx-abi/cxx-abi#170. Let's discuss the ABI related issue there.