eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.57k stars 373 forks source link

Bindgen generating rust binding failed. #879

Open ZhenshengLee opened 2 years ago

ZhenshengLee commented 2 years ago

Required information

I am using rust bindgen to generate rust binding based on iceoryx-binding-c.

And found that the struct iox_notification_info_t defined here directly refers to hpp file iceoryx_posh/include/iceoryx_posh/popo/notification_info.hpp with class NotificationInfo

https://github.com/eclipse-iceoryx/iceoryx/blob/abca825cf4c56de44b548128a8bb4d917d456869/iceoryx_binding_c/include/iceoryx_binding_c/notification_info.h#L25

Observed result or behaviour:

The c-binding is not pure.

given wraper.h

#include "iceoryx_binding_c/api.h"

rust bindgen outputs error msgs as follows:

cargo:rerun-if-changed=wrapper.h

  --- stderr
  /usr/local/include/iceoryx_binding_c/wait_set.h:53:28: error: unknown type name 'iox_notification_info_t'
  /usr/local/include/iceoryx_binding_c/wait_set.h:66:22: error: unknown type name 'iox_notification_info_t'
  /usr/local/include/iceoryx_binding_c/wait_set.h:53:28: error: unknown type name 'iox_notification_info_t', err: true
  /usr/local/include/iceoryx_binding_c/wait_set.h:66:22: error: unknown type name 'iox_notification_info_t', err: true
  thread 'main' panicked at 'Unable to generate bindings: ()', build.rs:37:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected result or behaviour:

api expoting normally.

class NotificationInfo should be rewrite to the struct like this

https://github.com/eclipse-iceoryx/iceoryx/blob/abca825cf4c56de44b548128a8bb4d917d456869/iceoryx_binding_c/include/iceoryx_binding_c/wait_set.h#L31

Conditions where it occurred / Performed steps:

Not export c api in iceoryx_binding_c/wait_set.h make error msgs disapear.

elBoberido commented 2 years ago

@ZhenshengLee I'm not sure why bindgen doesn't have issues with cpp2c_WaitSet since that is also a C++ class. Maybe because it is located in iceoryx_binding_c instead of iceoryx_posh.

Regarding the Rust wrapper. I've started one a long time ago but didn't fully port it to iceoryx v1.0. It's here https://github.com/elBoberido/iceoryx-rs. The plan is to move it at some time to eclipse-iceoryx. It already has a higher abstraction than what you get from the output of bindgen and uses the untyped C++ API. Would it be an option for you to continue with this approach?

ZhenshengLee commented 2 years ago

@ZhenshengLee I'm not sure why bindgen doesn't have issues with cpp2c_WaitSet since that is also a C++ class. Maybe because it is located in iceoryx_binding_c instead of iceoryx_posh.

Regarding the Rust wrapper. I've started one a long time ago but didn't fully port it to iceoryx v1.0. It's here https://github.com/elBoberido/iceoryx-rs. The plan is to move it at some time to eclipse-iceoryx. It already has a higher abstraction than what you get from the output of bindgen and uses the untyped C++ API. Would it be an option for you to continue with this approach?

There are demangled symbols(c symbols) and mangled symbols(cpp symbols) in libiceoryx_binding_c.so.

In fact, this iceoryx_binding_c is not pure and is much more like a c style cpp api, which makes it simple to use for programmers who prefer c, and they are using the c like cpp language, not pure c.

So, this kind of c_binding api with headers cannot be used by rust bindgen tool to automatically generate pure c symbols and rust binding.

But bindgen do have partial support to generate cpp symbols and their rust bindings, only with a subset of cpp features.

I'll give it a try to use bindgen to generate cpp symbols of iceoryx_binding_c.

After that, I will use your repo, and make rust api by hand.

elBoberido commented 2 years ago

You can of course create a patch for iceoryx and make it a pure C API. We just didn't encounter this problems until now since it seems the C compiler are not as strict as bindgen

Edit: Would do it by myself, but I'm currently just too busy

ZhenshengLee commented 2 years ago

You can of course create a patch for iceoryx and make it a pure C API. We just didn't encounter this problems until now since it seems the C compiler are not as strict as bindgen

Edit: Would do it by myself, but I'm currently just too busy

Actually I made a patch https://github.com/ZhenshengLee/iceoryx/commit/e65780d2002beb5f79cabcd95278d5561988775b to make it a c style cpp api to make it easy to be accepted by c++ compiler and rust bindgen.

Please see iceoryx-rs-c

As for now the publisher example works.

elBoberido commented 2 years ago

I had a look at your patch but I think it's not really correct since a C++ compiler is now necessary to use the C binding. Did you try to rewrite the NotificationInfo struct like cpp2c_WaitSet?

ZhenshengLee commented 2 years ago

I had a look at your patch but I think it's not really correct since a C++ compiler is now necessary to use the C binding. Did you try to rewrite the NotificationInfo struct like cpp2c_WaitSet?

It's not about only a symbol like NotificationInfo.

Rust bindgen uses clang to preprocess the header files, and generate binding.rs.

The binding.rs connects rust function with c(demangled) or cpp(mangled) functions(symbols).

Using bindgen you can select c or cxx symbols.

https://github.com/ZhenshengLee/iceoryx-rs-c/blob/master/doc/bindings_c_read_only.rs

extern "C" {
    pub fn iox_runtime_init(name: *const ::std::os::raw::c_char);
}

or

https://github.com/ZhenshengLee/iceoryx-rs-c/blob/master/doc/bindings_cpp_read_only.rs

extern "C" {
    #[link_name = "\u{1}_Z16iox_runtime_initPKc"]
    pub fn iox_runtime_init(name: *const ::std::os::raw::c_char);
}

Well, now in the main repo, you can check the symbols in libiceoryx_binding_c.

nm -a libiceoryx_binding_c.a | grep "iox_runtime_init"
00000000000000b0 T iox_runtime_init
# or .so

nvidia@Xavier:/usr/local/lib$ nm -a libiceoryx_binding_c.a | grep "runtime"
0000000000000128 T iox_node_get_runtime_name
                 U _ZN3iox7runtime11PoshRuntime10createNodeERKNS0_12NodePropertyE
                 U _ZN3iox7runtime11PoshRuntime11getInstanceEv
                 U _ZN3iox7runtime12NodePropertyC1ERKNS_3cxx6stringILm100EEEm
                 U _ZN3iox7runtime4NodeC2EPNS0_8NodeDataE
# ...

Bindings.rs generated by bindgen cannot handle that when there are both c and cpp symbols in a lib.

And undefined references error would happen, because it will find PoshRuntime c symbol which is a c++ symbols in your lib.

So I choose to generate cpp style binding.rs, which is a easier way.

elBoberido commented 2 years ago

@ZhenshengLee I think the proper solution here would be to have the extern "C" { ... } in the header file itself instead of just in the source file which includes the header. This ensures that there is no name mangling, no matter if a C or C++ compiler is used to compile a source file which includes this header. To test my theory, could wrap all the included header in wrapper.h in a extern "C" { ... } block and check if bindgen can still generate the bindings?

ZhenshengLee commented 2 years ago

I give it a test to your theory, and rustc bindgen will treat all api as demangled symbol, and output undefined reference error, like this.

/usr/bin/ld: /usr/local/lib/libiceoryx_binding_c.a(c_runtime.cpp.o): in function `iox_runtime_get_instance_name':
c_runtime.cpp:(.text+0x18b): undefined reference to `iox::runtime::PoshRuntime::getInstance()'
 /usr/bin/ld: c_runtime.cpp:(.text+0x196): undefined reference to `iox::runtime::PoshRuntime::getInstanceName() const'

@elBoberido Let's look at https://github.com/eclipse-iceoryx/iceoryx/blob/1fed2d6292cb070f58162fdcd70ee5e8907f29b9/iceoryx_binding_c/source/c_runtime.cpp#L50

This symbol is defined in a hpp file, so it must be a mangled symbol.https://github.com/eclipse-iceoryx/iceoryx/blob/1fed2d6292cb070f58162fdcd70ee5e8907f29b9/iceoryx_binding_c/source/c_runtime.cpp#L18

zs@zs-3630:/usr/local/lib$ nm -a ./libiceoryx_binding_c.a | grep "PoshRuntime"
                 U _ZN3iox7runtime11PoshRuntime10createNodeERKNS0_12NodePropertyE
                 U _ZN3iox7runtime11PoshRuntime11getInstanceEv
                 U _ZN3iox7runtime11PoshRuntime11getInstanceEv

That is the detail of the point I mentioned before.

There are demangled symbols(c symbols) and mangled symbols(cpp symbols) in libiceoryx_binding_c.so.

It's not about only a symbol like NotificationInfo.

Thanks.

mossmaurice commented 2 years ago

Closing this. @ZhenshengLee Feel free to re-open if this problem persists.

elBoberido commented 2 years ago

@mossmaurice I think we either need to reopen it or create a new issue to wrap the content of the header in an extern "C" block. With the current approach one cannot include the header from the binding_c into a C++ project (yes, it's weird but people do that) without wrapping the header itself into extern "C" which is nothing you would usually do and from my point of view has the same severity as if you do not include all the header you need and rely on the user of your header to include those.

cc @elfenpiff

elfenpiff commented 2 years ago

@mossmaurice @elBoberido I explicitly intended that you have to add extern "C" when someone would like to include a header from the binding_c the reason is: It is C. It is a C header and from the C binding. If you would like to use that specific header in a C++ project these are the rules.

I am opened for suggestions but I doubt that we can get rid of this.

Here for clarification, I found it somewhere in the internet: You need to use extern "C" in C++ when declaring a function that was implemented/compiled in C. The use of extern "C" tells the compiler/linker to use the C naming and calling conventions, instead of the C++ name mangling and C++ calling conventions that would be used otherwise.

elfenpiff commented 2 years ago

Here is a nice article to this topic: https://arne-mertz.de/2018/10/calling-cpp-code-from-c-with-extern-c/

ZhenshengLee commented 2 years ago

Here is a nice article to this topic: https://arne-mertz.de/2018/10/calling-cpp-code-from-c-with-extern-c/

This pattern is great.

extern "C" {
  #include "foo.h"
}

but this is the issue.

extern "C" {
  #include "foo.hpp"
}

I doubt that we can get rid of this.

for example https://github.com/eclipse-iceoryx/iceoryx/blob/a8c40c4688b8179e51f95b79dacfb5d7bc4b0b55/iceoryx_binding_c/source/c_runtime.cpp#L18

elBoberido commented 2 years ago

@elfenpiff nice argument but I have two counter arguments

  1. Booo! That's stupid ;)
  2. Although isocpp has this guideline for including non system headers, there is another guideline when we are the owner of the header. In this case it is preferred to have the content of the header wrapped in extern "C" instead of the include

So it's 2:1 for the wrapping of the content. It's your turn again :)

BH1SCW commented 2 years ago

Hi maintainer: It seems that this bug still not be fixed yet?

/usr/bin/ld: /usr/local/lib/libiceoryx_binding_c.a(c_subscriber.cpp.o): relocation R_X86_64_PC32 against symbol `_ZN3iox12ErrorHandler7handlerE' can not be used when making a shared object; recompile with -fPIC /usr/bin/ld: final link failed: bad value collect2: error: ld returned 1 exit status make[2]: [src/core/CMakeFiles/ddsc.dir/build.make:2340: lib/libddsc.so.0.10.0] Error 1 make[1]: [CMakeFiles/Makefile2:735: src/core/CMakeFiles/ddsc.dir/all] Error 2 make: *** [Makefile:152: all] Error 2

Thanks

ZhenshengLee commented 2 years ago

@BH1SCW Actually this issue was closed as rejected.

It seems that this bug still not be fixed yet?

No, the official tool rust-bindgen from rust is not supported by official iceoryx.

/usr/bin/ld: /usr/local/lib/libiceoryx_binding_c.a(c_subscriber.cpp.o): relocation R_X86_64_PC32 against symbol `_ZN3iox12ErrorHandler7handlerE' can not be used when making a shared object; recompile with -fPIC

the issue you described is the fact that there are mangled and pure symbols mixed in libiceoryx_binding_c.a

Actually I made a patch https://github.com/ros2middleware/iceoryx/commit/e65780d2002beb5f79cabcd95278d5561988775b to make it a c style cpp api to make it easy to be accepted by c++ compiler and rust bindgen. As for now the publisher example works.

this patch e65780d2002beb5f79cabcd95278d5561988775b of iceoryx change all symbols become mangled in libiceoryx_binding_c.a

Please see iceoryx-rust

see the rust-bindgen usage if interested. https://github.com/ZhenshengLee/iceoryx-rust/blob/848bbbb0a9d3f27781b46db1c7db34e4421dc599/build.rs#L27-L35

elfenpiff commented 2 years ago

@BH1SCW

From the first look it seems like it is maybe a different issue

/usr/bin/ld: /usr/local/lib/libiceoryx_binding_c.a(c_subscriber.cpp.o): 
  relocation R_X86_64_PC32 against symbol `_ZN3iox12ErrorHandler7handlerE' 
  can not be used when making a shared object; recompile with -fPIC

We compile every library and application with -fPIC since we require position independent code and it looks like this is missing here. Could you try to add this flag and retry again if it solves your issue.

But if you are looking for a nice rust API for iceoryx you can also look at https://github.com/eclipse-iceoryx/iceoryx-rs where @elBoberido is currently creating one which we intend to release in the next weeks.

BH1SCW commented 2 years ago

@ZhenshengLee @elfenpiff

Thanks for the quick response!

I found a workaround :

Add : set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC") to: iceoryx_binding_c/CMakeLists.txt iceoryx_hoofs/CMakeLists.txt iceoryx_posh/CMakeLists.txt

there are 3 file related to the error : /usr/bin/ld: /usr/local/lib/libiceoryx_binding_c.a(c_subscriber.cpp.o): relocation R_X86_64_PC32 against symbol _ZN3iox12ErrorHandler7handlerE' can not be used when making a shared object; recompile with -fPIC /usr/bin/ld: /usr/local/lib/libiceoryx_posh.a(service_description.cpp.o): relocation R_X86_64_PC32 against symbol_ZTTNSt7__cxx1118basic_stringstreamIcSt11char_traitsIcESaIcEEE@@GLIBCXX_3.4.21' can not be used when making a shared objec /usr/bin/ld: /usr/local/lib/libiceoryx_hoofs.a(logger.cpp.o): relocation R_X86_64_PC32 against symbol `_ZSt4cerr@@GLIBCXX_3.4' can not be used when making a shared object; recompile with -fPIC

BH1SCW commented 2 years ago

I am not sure if this is a good patch for submitting or not :)

ZhenshengLee commented 2 years ago

I found a workaround :

Add : set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")

OK, this should be another issue about compiling.

I am not sure if this is a good patch for submitting or not :)

Agreed, so the problem is open for the iceoryx team.

elfenpiff commented 2 years ago

@BH1SCW Thank you very much for finding the fix!

I created a PR, could you please take a look at #1420 and checkout if this fix solves your issue.

The reason I adjusted it myself instead of adding

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")

was

  1. We have CMake macros in place which setup all libraries and executables in a unified fashion and this would be the place for this setting, see iox_add_library and iox_add_executable
  2. When you are setting a global cmake variable then this -fPIC flag is also added to every project which depends on iceoryx and you may force a setting there which is not wished. This can easily be solved by setting it for a target explicitly.
  3. Setting CMake flags directly does not work compiler- and platform-independent. MSVC in windows for instance has there completely different settings so the best approach is to set_target_properties with POSITION_INDEPENDENT_CODE.

I thought it was quicker when I fix it myself then to let you go through our stony CMake path packed with obstacles but I wanted to give you as much insight as possible so the next cmake fix is yours to do :smile:

elBoberido commented 2 years ago

@elfenpiff I'm still in favor of the wrapping :)

BH1SCW commented 2 years ago

@BH1SCW Thank you very much for finding the fix!

I created a PR, could you please take a look at #1420 and checkout if this fix solves your issue.

Patch verified, works like a charm.

The reason I adjusted it myself instead of adding

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")

was

  1. We have CMake macros in place which setup all libraries and executables in a unified fashion and this would be the place for this setting, see iox_add_library and iox_add_executable
  2. When you are setting a global cmake variable then this -fPIC flag is also added to every project which depends on iceoryx and you may force a setting there which is not wished. This can easily be solved by setting it for a target explicitly.
  3. Setting CMake flags directly does not work compiler- and platform-independent. MSVC in windows for instance has there completely different settings so the best approach is to set_target_properties with POSITION_INDEPENDENT_CODE.

Thanks for guiding me.

I thought it was quicker when I fix it myself then to let you go through our stony CMake path packed with obstacles but I wanted to give you as much insight as possible so the next cmake fix is yours to do 😄

yeah~~~