Plugin Registration when using LLVM's libc++ in lieu of stdc++

[mikebauer]

Starting this primarily as a discussion since this might be a bit of a weird use case. I've been trying to link a unit test using rosbag2_ccp::Reader with clang + lld with llvm's libc++ implementation of the standard library. I've run into problems which might be related to Issue #78. In particular, at runtime, this dynamic_cast results in a nullptr due (approximately) to the mechanism discussed here under "dynamic_cast, throw, typeid don't work with shared libraries". I think this is due to the way typeinfo is handled by default in libc++. I'm currently using the clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz binary release for this and I've been solving this problem so far by using the following patch for mvukov/rules_ros2:

diff --git a/repositories/class_loader.BUILD.bazel b/repositories/class_loader.BUILD.bazel
index 7410f0b..bb27e0d 100644
--- a/repositories/class_loader.BUILD.bazel
+++ b/repositories/class_loader.BUILD.bazel
@@ -23,6 +23,11 @@ cc_library(
     srcs = ["class_loader.so"],
     hdrs = glob(["include/class_loader/*.hpp"]),
     copts = ["-std=c++17"],
+    # Ensure that we export these types from linked executables so that
+    # dynamic_casts work.
+    linkopts = [
+       "-Wl,--export-dynamic-symbol=\"_ZTIN12class_loader4impl18AbstractMetaObject*\""
+    ],
     includes = ["include"],
     visibility = ["//visibility:public"],
     deps = [

I think this works because it ensures type_info objects needed for the aforementioned dynamic_cast (i.e. class_loader::impl::AbstractMetaObject<T> for any T) are included in the dynamic symbols table of any executable that links @ros2_class_loader//:class_loader. This allows the dynamic cast to work correctly, but I can't help but feel like there must be a better solution to this problem.

If I instead link the standard library (i.e. -lstdc++), I can verify based on the typeinfo header that string comparison is being used for type_info objects (rather than pointer comparison) so the dynamic_cast appears to just work in this case.

Apologies for my limited understanding of the underlying issues. I'm happy to put together an example to reproduce this if folks would like.

[mikebauer]

I think this is actually very similar to Issue #78. Here's my theory: Because rosbag2_storage is being statically linked to my executable and rosbag2_storage_mcap_plugin, both were ending up with their own versions of AbstractMetaObject<rosbag2::storage_interfaces::ReadWriteInterface>. Another solution is therefore to do what @ahans did to fix #78 and force dynamic linking for rosbag2_storage:

diff --git a/repositories/rosbag2.BUILD.bazel b/repositories/rosbag2.BUILD.bazel
index 0a3db1b..a9c196c 100644
--- a/repositories/rosbag2.BUILD.bazel
+++ b/repositories/rosbag2.BUILD.bazel
@@ -15,15 +15,15 @@ load("@com_github_mvukov_rules_ros2//ros2:plugin.bzl", "ros2_plugin")
 load("@rules_python//python:defs.bzl", "py_library")
 load("@rules_ros2_pip_deps//:requirements.bzl", "requirement")

-ros2_cpp_library(
-    name = "rosbag2_storage",
+ros2_cpp_binary(
+    name = "rosbag2_storage.so",
     srcs = glob([
+        "rosbag2_storage/include/**/*.hpp",
         "rosbag2_storage/src/**/*.cpp",
         "rosbag2_storage/src/**/*.hpp",
     ]),
-    hdrs = glob(["rosbag2_storage/include/**/*.hpp"]),
     includes = ["rosbag2_storage/include"],
-    visibility = ["//visibility:public"],
+    linkshared = True,
     deps = [
         "@ros2_pluginlib//:pluginlib",
         "@ros2_rcpputils//:rcpputils",
@@ -32,6 +32,17 @@ ros2_cpp_library(
     ],
 )

+ros2_cpp_library(
+    name = "rosbag2_storage",
+    srcs = ["rosbag2_storage.so"],
+    hdrs = glob(["rosbag2_storage/include/**/*.hpp"]),
+    includes = ["rosbag2_storage/include"],
+    visibility = ["//visibility:public"],
+    deps = [
+        ":rosbag2_storage.so",
+    ],
+)
+
 ros2_plugin(
     name = "rosbag2_storage_default_plugins",
     srcs = glob([

Not sure if there are other (direct or indirect) dependencies of class_loader which may also be similarly affected.

[mvukov]

Thanks for all the details. I'm not that versed with linkers and gcc/clang impl details. BTW, I also saw this thing a while back 🤯 https://github.com/ros2/rosbag2/blob/96c11b073c9bf56fb35586205b941e14f8f15b6a/rosbag2_py/test/common.py#L20 .

Anyway, just for my understanding: for your use-case we need both diffs in place, right? Or only the first one? Speaking out loud, would -Wl,-export-dynamic suffice (instead of having to specify the concrete symbol)? (perhaps in combination with -fvisibility=hidden).

I am open to alternatives. For me it's also fine to to try out https://bazel.build/reference/be/c-cpp#cc_shared_library instead of cc_binary+cc_library hack.

Whatever comes out of this, let's set up CI for clang+libc++ as well.

[mikebauer]

Hey Milan,

Sorry for the delay in getting back to you. From my observations, either diff is individually enough for things to work. The first works by making sure the symbols are visible in the dynamic symbols table of the executable, so -Wl,-export-dynamic would also suffice in that case, although I think it would result in larger binaries. I'm partial to the second approach myself since linking robag2_storage as a shared object file seems to be closer to what the developers of that library would expect (given that that's how it's built/shipped in the binary distributions). I've consequently added a patch for myself for now that does that, but I can probably put up a PR for this repo next weekend if you think that's the right call.

Thanks for pointing out cc_shared_library(). I'll give that a try. 🙂

[mvukov]

Hi Mike,

Both options are OK for me. I am kind of leaning towards the first one if we can circumvent the ugliness of the exported symbol; ATM looks like less maintenance longer term. But I am fine with the second option as well; if we go this route we should add docs in this repo with some best practices.

Anyway, please paste here what exact link flags you use for clang+libc++ combo such that I can experiment on my side as well. Thanks in advance.

[mvukov]

Just FYI, I'm working on an alternative in #185. This PR, still WIP, should resolve your issues with libc++ as well. All tests pass locally for that PR, CI complains about a flaky test (should be fixed in a separate PR).

BTW, cc_shared_library won't work for the mentioned case because it doesn't propagate CcInfo.