Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linear DS set_gain() not implemented - inlined public functions in source file may be optimised away #75

Closed
eeberhard opened this issue Mar 24, 2021 · 3 comments · Fixed by #76
Labels
bug Something isn't working

Comments

@eeberhard
Copy link
Member

This one had me stuck for quite a while... I wanted to adapt the gains of a Linear DS after construction, so that I can change the behaviour at runtime. So I used the set_gain() function. For some reason, I was having linking errors when trying to do it.

The thing is, I knew my usage was correct - I added a test case to control_libraries to set the gain of a constructed Linear DS. There, it was working flawlessly. Then I had a thought. The only difference between the test case and my usage was that the library was built in Debug rather than Release. And, I saw the problematic function was specified inline even though it was implemented in the source file.


Steps to reproduce:

  • Make a simple test executable that constructs a Linear ds and then sets the gains afterwards
#include <state_representation/space/cartesian/CartesianState.hpp>
#include <dynamical_systems/Linear.hpp>

int main(int, char**) {
  state_representation::CartesianState attractor("A");
  dynamical_systems::Linear<state_representation::CartesianState> linear(attractor);

  linear.set_gains(1.0);

  std::vector<double> gains = {1, 2, 3, 4, 5, 6};
  linear.set_gain(gains);

  return 0;
}
  • Build control libraries dynamical systems with -DCMAKE_BUILD_TYPE=Release
  • Build the test executable
  • The linking will fail with undefined function Linear::set_gain() on both overloaded variants.
  • Rebuild control libraries dynamical systems with -DCMAKE_BUILD_TYPE=Debug
  • Rebuild the test executable
  • The linking should succeed and the build completes.

My theory is that because these public functions are using the inline directive, even though they are defined inside the source file, the compiler optimises their definitions away. After all, if it's only being used locally in the source file, there's no point keeping the definition available to other translation units. In debug release that optimisation may not happen, so the function definition is still available during the linking step.

@eeberhard eeberhard added the bug Something isn't working label Mar 24, 2021
@buschbapti
Copy link
Contributor

Oh wow that is a nasty one. Well obviously the error is that there should not be declared inline in the source file. Despite the fact that it does not make any sense for them to be, if not in the header, I didn't know it could have such drastic consequences. At least this is an easy fix and it is very good that we are now aware of this.

eeberhard added a commit that referenced this issue Mar 25, 2021
* Public functions Linear::set_gain were defined in source,
but declared inline. The optimisation level of the compiler
could then remove these definitions entirely, causing
linker errors for users down the line.

This should close issue #75
@eeberhard eeberhard linked a pull request Mar 25, 2021 that will close this issue
@eeberhard
Copy link
Member Author

Should be resolved by PR #76, will confirm with a downstream test before closing this issue

@eeberhard
Copy link
Member Author

I verified with an external test built in Release configuration and it resolved the issue as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants