epfl-lasa/control-libraries

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

eeberhard opened this issue · 3 comments

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.

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.

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

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