You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/01/24 07:38:21 UTC
[GitHub] [arrow] kou opened a new pull request, #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
kou opened a new pull request, #33847:
URL: https://github.com/apache/arrow/pull/33847
### Rationale for this change
Some base type classes don't have hidden method implementations. It may define these classes in each translation unit. It may cause one-definition-rule violation with `-flto`.
### What changes are included in this PR?
Define at least one hidden method to prevent defining these base type classes in each translation unit.
### Are these changes tested?
How to reproduce:
```bash
CFLAGS="-flto" CXXFLAGS="-flto cmake ...
cmake --build ...
```
This reports link errors without this change.
### Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087316762
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1086710255
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
You probably don't need to define the destructor separately?
```suggestion
~FixedWidthType() override = default;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 merged pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 merged PR #33847:
URL: https://github.com/apache/arrow/pull/33847
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087389699
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33847:
URL: https://github.com/apache/arrow/pull/33847#issuecomment-1401500669
@github-actions crossbow submit test-r-rhub-debian-gcc-devel-lto-latest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1086710255
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
You probably don't need to define the destructor separately?
```suggestion
~FixedWidthType() override = default;
```
(see https://stackoverflow.com/questions/70387962/does-declaring-a-constructor-default-in-a-header-file-break-the-odr)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087319273
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1088951185
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1086237224
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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:
```console
$ 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:
```console
$ 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:
```text
[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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33847:
URL: https://github.com/apache/arrow/pull/33847#issuecomment-1406549088
Benchmark runs are scheduled for baseline = 108a0286073a38ba8be59c6c01a2cc408f61fccd and contender = 24da3be9560c5ee4eddbd62a815af553902504b8. 24da3be9560c5ee4eddbd62a815af553902504b8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/6adafce4f3984604b762ecf0c0e3ddee...99b153b64308476b834e0bb63427bd12/)
[Failed :arrow_down:0.48% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/242f4eb8871e421882eb0bbce837bf85...c1ef47e093014d7d9359928db3358e19/)
[Failed :arrow_down:1.85% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c3f0723ada874c16bbf9764bdd24eff5...bd81674450af40b8a10b190d271a0b79/)
[Finished :arrow_down:0.31% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/290d65eebb504c6da72aab53102c0940...0520da6df5f247228cd4b22ce98ca0d6/)
Buildkite builds:
[Finished] [`24da3be9` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2278)
[Failed] [`24da3be9` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2303)
[Failed] [`24da3be9` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2273)
[Finished] [`24da3be9` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2296)
[Finished] [`108a0286` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2277)
[Failed] [`108a0286` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2302)
[Failed] [`108a0286` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2272)
[Finished] [`108a0286` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2295)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #33847:
URL: https://github.com/apache/arrow/pull/33847#issuecomment-1402081906
Thank you @kou!
I don't know enough about LTO or how/why this is a fix to say that this should or should not be defensively merged. Not having warnings during LTO linking makes my life easier (I don't have to figure out how to silence that nightly job or to attempt justifying anything to CRAN; however, I don't know enough about the details to make a recommendation otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1085362914
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
Curious why it causes ODR violation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #33847:
URL: https://github.com/apache/arrow/pull/33847#issuecomment-1401544193
@paleolimbot This is another approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1086697921
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
I don't know why it happens. Clang doesn't complain ODR issue.
cc @pitrou, @westonpace, @lidavidm for comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087020212
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
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](https://lld.llvm.org/missingkeyfunction.html):
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087404291
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #33847: GH-33701: [C++] Add support for LTO (link time optimization) build
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on code in PR #33847:
URL: https://github.com/apache/arrow/pull/33847#discussion_r1087380589
##########
cpp/src/arrow/type.h:
##########
@@ -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;
Review Comment:
I think this change is okay. And I wonder how you find this solution, it's not obvious from the error log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org