Multiple Definitions

[pointbazaar]

Using sdbusplus i am having problems with the implementations present in the .hpp files of the library. Somewhat Minimal Example:

#include <sdbusplus/async.hpp>

extern void foo();

int main(){ return 1; }

#include <sdbusplus/async.hpp>

void foo(){ }

export INCLUDES ="-I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include/ -I/usr/local/include"

export LINK = -Lbuild/ -lsdbusplus

cpp:
    g++ --std=c++20 *.cpp ${INCLUDES} ${LINK}

which is causing me

g++ --std=c++20 *.cpp "-I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include/ -I/usr/local/include" -Lbuild/ -lsdbusplus
/usr/bin/ld: /tmp/cc4fU4mh.o: in function `sdbusplus::async::match::next()':
minimal.cpp:(.text+0xc): multiple definition of `sdbusplus::async::match::next()'; /tmp/cco2Wp5v.o:foo.cpp:(.text+0xc): first defined here
collect2: error: ld returned 1 exit status
make: *** [Makefile:7: cpp] Error 1

(multiple definitions)

So my understanding is that Header files (.hpp) in C++ are for declaring classes, structs, function signatures and not implementations?

I could be wrong here, so please correct me if thats the case.

However in include/sdbusplus/async/match.hpp i find

auto match::next() noexcept                                                                                                                                                             
{
     return match_ns::match_sender(*this);
}

which is an implementation. So how is a user of this library supposed to handle this?

Are there are linker flags or such that must be passed or any other obscure means to get it to work? Because i wanted to implement my project in multiple files and need to include some headers in more than one file.

[williamspatrick]

Neat to hear someone is playing with this. I'll admit there haven't been a lot of users of this async code yet and as such there are probably bugs and APIs that could be improved.

This sounds like a bug. There are two match::next, one which is templated and one which is not. We probably need an inline or static on the one you are having issues with.

[pointbazaar]

@williamspatrick Yeah i am new to this library, so i am probably not using it in the most idiomatic way.

If you want, i can try to create a PR to try and fix this across all the headers, if there should be any others like this. So every header can be included without causing any linking problems because of multiple definitions.

[williamspatrick]

@williamspatrick Yeah i am new to this library, so i am probably not using it in the most idiomatic way.

I'm not sure anything is idiomatic at this point. Feel free to ask me questions and/or provide suggestions. We do have a Discord for the OpenBMC project if you want more real-time communication.

If you want, i can try to create a PR to try and fix this across all the headers, if there should be any others like this. So every header can be included without causing any linking problems because of multiple definitions.

Two hang-ups for new contributors:

1. We don't accept Github PRs. All development is done on a Gerrit server (gerrit.openbmc.org) and mirrored here after merge.
2. The organization, unfortunately, requires a CLA in order to accept contributions. See https://github.com/openbmc/docs/blob/43421bdd06dece37b2acc4c656d7839bfedcc681/CONTRIBUTING.md#starting-out . You can sign an ICLA as an individual or possibly a CCLA for your corporation (depending on who's behalf you are doing this work and the associated open source policies).

[pointbazaar]

@williamspatrick found out employer has CCLA, i just created an account on your gerrit aswell.

Having a look at the headers, i fear that the Changes i am trying to make are beyond my C++ skills at this point. The Pattern i am having Issues with is pervasive throughout most of sdbusplus Headers.

The Pattern is:

• Declaration and Definition of functions in .hpp files with auto return type and template.

If it were just auto, then one could simply substitute the type and move the function into .cpp file, but in many cases it is not possible, as the template permits multiple return types.

e.g. message::unpack in include/sdbusplus/message.hpp

So my understanding is that the templates get resolved and then one can end up with multiple identical implementations of essentially the same function in different compilation units.

So my question is how does e.g. <vector> solve this.

My workaround for now is -z muldefs to let g++/ld ignore this.

Regarding your comments about inline and static, wouldn't they both cause the code in that function to be duplicated? I am not experienced in C++ so i can't say for sure. Inlining would definitely duplicate code so that's surely not what is desired for users of this library?

Looking forward to your responses.

[williamspatrick]

@williamspatrick found out employer has CCLA, i just created an account on your gerrit aswell.

Great!

Having a look at the headers, i fear that the Changes i am trying to make are beyond my C++ skills at this point. The Pattern i am having Issues with is pervasive throughout most of sdbusplus Headers.

The Pattern is:

• Declaration and Definition of functions in .hpp files with auto return type and template.

I don't think this is as pervasive of a problem as you are imagining. Everything except the newer async code is pretty widely used at this point and I've not seen any problems.

Template functions are 100% not an issue due to C++ language's ODR:

There can be more than one definition in a program of each of the following: class type, enumeration type, inline function, inline variable (since C++17), [templated entity](https://en.cppreference.com/w/cpp/language/templates#Templated_entity) (template or member of template, but not full [template specialization](https://en.cppreference.com/w/cpp/language/template_specialization)), as long as all of the following is true:

I'm fairly certain the issues you are seeing should only affect non-template functions defined in headers.

Regarding your comments about inline and static, wouldn't they both cause the code in that function to be duplicated?

Inline functions are another example where the ODR is satisfied, so that's why it can be used to alleviate the problem. One thing to keep in mind is that just because a function is marked as 'inline' doesn't mean that the compiler is required to inline it (many compilers provide some sort of 'always_inline' specifier as well).

The case you originally reported, match::next(), the function is essentially a syntatic-sugar around a constructor. Non-inlining the function probably gives more overhead since you'd be outlining a function with the same number of arguments as the constructor which in turn just makes a call to the constructor. Small functions like this are perfectly acceptable to inline.

If you could, can you try to set that function as inline and let me know if there are any other cases you're explicitly running into?

[pointbazaar]

@williamspatrick using inline on the definitions does not work (still get multiple definitions), and __attribute__((always_inline)) results in compilation errors.

My use case is to

sdbusplus::message::message msg = co_await match.next();

inside a loop to listen for signals and handle them. I originally wanted to handle signals similar to how i register methods, but could not find an example that handled signals that way.

iface->register_method("methodName", MyClass::myfunction);

Is there a way to register a handler for signals or do i have to use a loop for this?

[williamspatrick]

Those are definately two very different ways of programming in sdbusplus.

The register_method is part of the boost ASIO code. I've not written or used that. It is very callback heavy. The co_await is using C++ coroutines. Currently the coroutine and boost stuff doesn't interoperate anyhow.

If you want a more callback-centric way of doing signal matches, take a look at bus/match.hpp. It has a constructor which takes a function callback.