ros-drivers / ros2_ouster_drivers

ROS2 Drivers for the Ouster OS-0, OS-1, and OS-2 Lidars
https://ouster.com/
Apache License 2.0
139 stars 79 forks source link

Fix the build under Clang #27

Closed rotu closed 4 years ago

rotu commented 4 years ago
ld.lld: error: undefined symbol: ros2_ouster::OusterDriver<OS1::OS1Sensor>::OusterDriver(rclcpp::NodeOptions const&)
>>> referenced by new_allocator.h:147 (/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:147)
>>>               CMakeFiles/ouster_driver.dir/src/main.cpp.o:(std::__shared_ptr<ros2_ouster::OusterDriver<OS1::OS1Sensor>, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<ros2_ouster::OusterDriver<OS1::OS1Sensor> >, rclcpp::NodeOptions&>(std::_Sp_alloc_shared_tag<std::allocator<ros2_ouster::OusterDriver<OS1::OS1Sensor> > >, rclcpp::NodeOptions&))

ld.lld: error: undefined symbol: ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
>>> referenced by main.cpp
>>>               CMakeFiles/ouster_driver.dir/src/main.cpp.o:(vtable for ros2_ouster::OusterDriver<OS1::OS1Sensor>)
>>> did you mean: ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
>>> defined in: libouster_driver_core.so
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [CMakeFiles/ouster_driver.dir/build.make:157: ouster_driver] Error 1
make[1]: *** [CMakeFiles/Makefile2:78: CMakeFiles/ouster_driver.dir/all] Error 2
make: *** [Makefile:141: all] Error 2
SteveMacenski commented 4 years ago

I don't understand why this is required.

rotu commented 4 years ago

Because it can't find the definition of the method. The constructor and destructor of the template method must be available in the header because it is a template class.

SteveMacenski commented 4 years ago

What's giving you that error? @tpanzarella can you reproduce? I've never seen this. I'm generally against placing any functional code in header files without a compelling reason.

rotu commented 4 years ago

I'm doing a build under clang 11, and surprised that you're not getting that error. When you have a template class, it doesn't automatically instantiates the function. So anything that instantiates or destroys that template class needs the function definition.

I agree - implementations don't generally belong in headers. But they do when the class is a template class.

SteveMacenski commented 4 years ago

That's what this file is for: https://github.com/SteveMacenski/ros2_ouster_drivers/blob/eloquent-devel/ros2_ouster/include/ros2_ouster/driver_types.hpp#L28 so that you're instantiating the class type with template parameters.

rotu commented 4 years ago

I think it's because main.cpp and ouster_driver.cpp both import that header. So the template class is getting instantiated in both compilation units (one per cpp file). Look at the error message again:

ld.lld: error: undefined symbol: ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
>>> referenced by main.cpp
>>> did you mean: ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
>>> defined in: libouster_driver_core.so

This leads me to believe this PR is just a bandaid on the issue - something needs to change to make the class exist in just one compilation unit.

Also, the template should be defined in a private header or source file, not a public one. Otherwise anyone including it will wind up instantiating it yet again.

tpanzarella commented 4 years ago

Hey sorry for jumping in here late guys -- it's late here, I'm on the East Coast. So, yes, I can reproduce this with clang however it does compile fine with gcc. I do not think that this PR has the correct fix to this issue. As you guys are alluding to above, the issue has to do with the explicit instantiation of the template. @SteveMacenski has done the instantiation in driver_types.hpp which is included by ouster_driver.cpp before the definition of the class. Per the C++ standard:

An explicit instantiation definition that names a class template specialization explicitly instantiates the class template specialization and is an explicit instantiation definition of only those members that have been defined at the point of instantiation.

So, if you move the explicit instantiation after the class definition it works fine. Here is a diff in my tree that will make this compile -- it is just me hacking around to make it work, not a "PR" per se:

tpanzarella@jelly $ git diff
diff --git a/ros2_ouster/include/ros2_ouster/driver_types.hpp b/ros2_ouster/include/ros2_ouster/driver_types.hpp
index 0486b57..c0afd96 100644
--- a/ros2_ouster/include/ros2_ouster/driver_types.hpp
+++ b/ros2_ouster/include/ros2_ouster/driver_types.hpp
@@ -25,8 +25,8 @@
 namespace ros2_ouster
 {

-template class ros2_ouster::OusterDriver<OS1::OS1Sensor>;
-using OS1Driver = ros2_ouster::OusterDriver<OS1::OS1Sensor>;
+  //template class ros2_ouster::OusterDriver<OS1::OS1Sensor>;
+  //using OS1Driver = ros2_ouster::OusterDriver<OS1::OS1Sensor>;

 }  // namespace ros2_ouster

diff --git a/ros2_ouster/src/main.cpp b/ros2_ouster/src/main.cpp
index b00216a..82ccca3 100644
--- a/ros2_ouster/src/main.cpp
+++ b/ros2_ouster/src/main.cpp
@@ -15,13 +15,16 @@

 #include "ros2_ouster/ouster_driver.hpp"
 #include "rclcpp/rclcpp.hpp"
-#include "ros2_ouster/driver_types.hpp"
+#include "ros2_ouster/OS1/OS1_sensor.hpp"
+//#include "ros2_ouster/driver_types.hpp"

 int main(int argc, char ** argv)
 {
   rclcpp::init(argc, argv);
   auto options = rclcpp::NodeOptions();
-  auto node = std::make_shared<ros2_ouster::OS1Driver>(options);
+  //auto node = std::make_shared<ros2_ouster::OS1Driver>(options);
+  auto node =
+    std::make_shared<ros2_ouster::OusterDriver<OS1::OS1Sensor>>(options);

   rclcpp::spin(node->get_node_base_interface());

diff --git a/ros2_ouster/src/ouster_driver.cpp b/ros2_ouster/src/ouster_driver.cpp
index c1bebfa..c7e74d9 100644
--- a/ros2_ouster/src/ouster_driver.cpp
+++ b/ros2_ouster/src/ouster_driver.cpp
@@ -232,4 +232,5 @@ void OusterDriver<SensorT>::getMetadata(
   response->metadata = toMsg(_sensor->getMetadata());
 }

+template class ros2_ouster::OusterDriver<OS1::OS1Sensor>;
 }  // namespace ros2_ouster

I'll add some additional color in a subsequent comment.

SteveMacenski commented 4 years ago

Its possible we can also just remove the template and just use an interface. Honestly, I forget off hand why I have both. If no one sees a clear reason, we might be able to chalk it up to a 3am coding session and we can just use an interface. I think I was trying to tie the processors to the laser type and never quite got there (and also that those processor types would likely be relevant for all subsequent units this driver supports). Or if we like the template, remove the interface.

I just want easy support for OS-0 and OS-2.

tpanzarella commented 4 years ago

I think in the short-term, a patch to get this to work on clang should be made. And that is just moving the explicit instantiation to the right spot. When it comes after the class definition, you can see that clang generates the proper symbols for the ctor and dtor in the shared object file, as you intended:

tpanzarella@jelly $ nm --demangle build/ros2_ouster/libouster_driver_core.so  | grep -i 'OusterDriver<OS1::OS1Sensor>::~'
000000000010d200 W ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
000000000010d120 W ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()
000000000010d120 W ros2_ouster::OusterDriver<OS1::OS1Sensor>::~OusterDriver()

tpanzarella@jelly $ nm --demangle build/ros2_ouster/libouster_driver_core.so  | grep -i 'OusterDriver<OS1::OS1Sensor>::OusterDriver'
000000000010bc50 W ros2_ouster::OusterDriver<OS1::OS1Sensor>::OusterDriver(rclcpp::NodeOptions const&)
000000000010bc50 W ros2_ouster::OusterDriver<OS1::OS1Sensor>::OusterDriver(rclcpp::NodeOptions const&)

Doing it there once and having the proper symbol available in the .so then allows us to remove the second explicit instantiation that was being done in main.cpp (by including driver_types.hpp) and we can link as per normal.

Back to your original comment @SteveMacenski removing the template is not strictly necessary from a technical standpoint. But it sounds like you want to reconsider some architecture, which is a different discussion. I'd need to think about that a bit to give any meaningful feedback. For now ... bed :-)

rotu commented 4 years ago

I do think an interface is a better choice, FWIW. Templates are cool but I think this code doesn’t need them. I might take another whack at this tomorrow.

tpanzarella commented 4 years ago

As a point of perspective, idiomatic C++ is trending more toward generic/functional interfaces vs. OO inheritance hierarchies -- OO/classes are still critical in terms of defining types, but, coupling them tightly via inheritance relationships has several downsides. With Concepts in C++20 (hopefully the ROS2 LTS after F-turtle will require it) generic interfaces will feel a lot less like duck typing (as they can at times today). Just a different perspective to consider.

Specific to this discussion, generally, when writing templated classes we don't want to have to rely on explicit instantiations as it feels like a loss of generality. However, in this context (a LiDAR driver), I just look at it as a registry.

You guys know all of this, but just putting the "non-interface" approach on the table for discussion.