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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to