Skip to content

GH-33701: [C++] Add support for LTO (link time optimization) build #33847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cyb70289 merged 1 commit into from
Jan 27, 2023
Merged

GH-33701: [C++] Add support for LTO (link time optimization) build #33847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions cpp/src/arrow/type.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,16 @@ std::string TypeHolder::ToString(const std::vector<TypeHolder>& types) {

// ----------------------------------------------------------------------

FixedWidthType::~FixedWidthType() {}

PrimitiveCType::~PrimitiveCType() {}

NumberType::~NumberType() {}

IntegerType::~IntegerType() {}

FloatingPointType::~FloatingPointType() {}

FloatingPointType::Precision HalfFloatType::precision() const {
return FloatingPointType::HALF;
}
Expand All @@ -478,6 +488,12 @@ std::ostream& operator<<(std::ostream& os,
return os;
}

NestedType::~NestedType() {}

BaseBinaryType::~BaseBinaryType() {}

BaseListType::~BaseListType() {}

std::string ListType::ToString() const {
std::stringstream s;
s << "list<" << value_field()->ToString() << ">";
Expand Down Expand Up @@ -589,6 +605,8 @@ std::string FixedSizeBinaryType::ToString() const {
return ss.str();
}

TemporalType::~TemporalType() {}

// ----------------------------------------------------------------------
// Date types

Expand Down
27 changes: 27 additions & 0 deletions cpp/src/arrow/type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,31 +288,46 @@ std::shared_ptr<DataType> GetPhysicalType(const std::shared_ptr<DataType>& type)
class ARROW_EXPORT FixedWidthType : public DataType {
public:
using DataType::DataType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~FixedWidthType() override;
Copy link
Contributor

@cyb70289 cyb70289 Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why it causes ODR violation.

Copy link
Member Author

@kou kou Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I don't know much about this but...

I tried to create a small example to reproduce this case but couldn't create it... So I explain what I observed:

If we don't have this, .o of a C++ code that has std::make_shared<arrow::XXXType> such as std::make_shared<arrow::ListType> has the following nm result:

$ nm --demangle src/arrow/CMakeFiles/arrow_objlib.dir/compute/kernels/scalar_cast_numeric.cc.o | grep 'vtable for arrow::FloatingPointType'
00000000 W vtable for arrow::FloatingPointType

W is a global weak symbol.

https://linux.die.net/man/1/nm

If lowercase, the symbol is local; if uppercase, the symbol is global (external).

"W"
"w"

The symbol is a weak symbol that has not been specifically tagged as a weak object symbol.

If we have this, the symbol becomes a undefined symbol:

$ nm --demangle src/arrow/CMakeFiles/arrow_objlib.dir/compute/kernels/scalar_cast_numeric.cc.o | grep 'vtable for arrow::FloatingPointType'
         U vtable for arrow::FloatingPointType

https://linux.die.net/man/1/nm

"U"

The symbol is undefined.

If there are multiple W vtable for arrow::FloatingPointType in our .o. linking libarrow.so with -flto is failed:

[650/1111] Linking CXX shared library debug/libarrow.so.1100.0.0
FAILED: debug/libarrow.so.1100.0.0 
: && /bin/c++ -fPIC -Wno-noexcept-type -flto -fdiagnostics-color=always  -Wall -Wno-conversion -Wno-sign-conversion -Wunused-result -fno-semantic-interposition -msse4.2  -g -Werror -O0 -ggdb -shared -Wl,-soname,libarrow.so.1100 -o debug/libarrow.so.1100.0.0 ...(**/*.o)... && :
cpp/src/arrow/type.h:313: error: virtual table of type ‘struct FloatingPointType’ violates one definition rule [-Werror=odr]
  313 | class ARROW_EXPORT FloatingPointType : public NumberType {
      | 
cpp/src/arrow/type.h:313:20: note: the conflicting type defined in another translation unit
  313 | class ARROW_EXPORT FloatingPointType : public NumberType {
      |                    ^
<built-in>: note: virtual method ‘__cxa_pure_virtual’
...

It seems that W and -flto is a bad combination. If we change W to U by defining a not-inlined deconstructor, we don't get the ODR error.

Do you notice anything from this observation?

Copy link
Contributor

@cyb70289 cyb70289 Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it happens. Clang doesn't complain ODR issue.
cc @pitrou, @westonpace, @lidavidm for comments.

Copy link
Member

@westonpace westonpace Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why adding that destructor changes W to U. You are defining a non-inline key function. Therefore the vtable is defined in only one place.

However, I don't understand why it is an ODR error. For example, in the clang docs:

When a class has no non-pure, non-inline, virtual functions, there is no key function, and the compiler is forced to emit the vtable in every translation unit that references the class. In this case, it is emitted in a COMDAT section, which allows the linker to eliminate all duplicate copies. This is still wasteful in terms of object file size and link time, so it’s always advisable to ensure there is at least one eligible function that can serve as the key function.

Copy link
Member Author

@kou kou Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this is only happen with GCC (GNU ld?).
I couldn't reproduce this with CC=clang CXX=clang++ CFLAGS="-flto" CXXFLAGS="-flto" cmake .... (W symbol is still generated but there is no link error.)

Is it acceptable that we add this change only for GCC and LTO?

Copy link
Contributor

@cyb70289 cyb70289 Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is okay. And I wonder how you find this solution, it's not obvious from the error log.

Copy link
Member Author

@kou kou Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a try and error...

There are some abstract classes for arrow::DataType family. Most of them are in error messages but arrow::DataType isn't shown. I noticed that arrow::DataType is U and others are W in nm --demangle result. I found that arrow::DataType::~DataType() is defined in type.cc but other abstract classes don't define their destructors. So I tried to define them.

Copy link
Contributor

@cyb70289 cyb70289 Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a class has no non-pure, non-inline, virtual functions, there is no key function, and the compiler is forced to emit the vtable in every translation unit that references the class. In this case, it is emitted in a COMDAT section, which allows the linker to eliminate all duplicate copies. This is still wasteful in terms of object file size and link time, so it’s always advisable to ensure there is at least one eligible function that can serve as the key function.

Does it mean if a class has virtual functions, it's recommended that at least one of them should be in .cc?
I find it hard to understand a class has no non-pure, non-inline, virtual functions.

Copy link
Member

@westonpace westonpace Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what it means. I think "wasteful" is too strong of a word though. I think the consequence should generally be minor enough that it isn't really a concern.

Copy link
Member

@pitrou pitrou Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't need to define the destructor separately?

Suggested change
~FixedWidthType() override;
~FixedWidthType() override = default;

(see https://stackoverflow.com/questions/70387962/does-declaring-a-constructor-default-in-a-header-file-break-the-odr)

Copy link
Member Author

@kou kou Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried = default. If we add = default, generated symbol is still W not U. It seems that we need to define the destructor separately for U.

};

/// \brief Base class for all data types representing primitive values
class ARROW_EXPORT PrimitiveCType : public FixedWidthType {
public:
using FixedWidthType::FixedWidthType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~PrimitiveCType() override;
};

/// \brief Base class for all numeric data types
class ARROW_EXPORT NumberType : public PrimitiveCType {
public:
using PrimitiveCType::PrimitiveCType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~NumberType() override;
};

/// \brief Base class for all integral data types
class ARROW_EXPORT IntegerType : public NumberType {
public:
using NumberType::NumberType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~IntegerType() override;
virtual bool is_signed() const = 0;
};

/// \brief Base class for all floating-point data types
class ARROW_EXPORT FloatingPointType : public NumberType {
public:
using NumberType::NumberType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~FloatingPointType() override;
enum Precision { HALF, SINGLE, DOUBLE };
virtual Precision precision() const = 0;
};
Expand All @@ -323,6 +338,9 @@ class ParametricType {};
class ARROW_EXPORT NestedType : public DataType, public ParametricType {
public:
using DataType::DataType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~NestedType() override;
};

/// \brief The combination of a field name and data type, with optional metadata
Expand Down Expand Up @@ -650,6 +668,9 @@ class ARROW_EXPORT DoubleType
class ARROW_EXPORT BaseBinaryType : public DataType {
public:
using DataType::DataType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~BaseBinaryType() override;
};

constexpr int64_t kBinaryMemoryLimit = std::numeric_limits<int32_t>::max() - 1;
Expand Down Expand Up @@ -893,6 +914,9 @@ class ARROW_EXPORT Decimal256Type : public DecimalType {
class ARROW_EXPORT BaseListType : public NestedType {
public:
using NestedType::NestedType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~BaseListType() override;
const std::shared_ptr<Field>& value_field() const { return children_[0]; }

std::shared_ptr<DataType> value_type() const { return children_[0]->type(); }
Expand Down Expand Up @@ -1209,6 +1233,9 @@ class ARROW_EXPORT DenseUnionType : public UnionType {
class ARROW_EXPORT TemporalType : public FixedWidthType {
public:
using FixedWidthType::FixedWidthType;
// This is only for preventing defining this class in each
// translation unit to avoid one-definition-rule violation.
~TemporalType() override;

DataTypeLayout layout() const override {
return DataTypeLayout(
Expand Down