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

Peculiar memory error #32

Open
wannesvanloock opened this issue Jul 10, 2019 · 2 comments
Open

Peculiar memory error #32

wannesvanloock opened this issue Jul 10, 2019 · 2 comments

Comments

@wannesvanloock
Copy link
Contributor

wannesvanloock commented Jul 10, 2019

I encountered strange behavior related to Eigen and nanoflann in cilantro. Consider the following example.

#include <cilantro/kd_tree.hpp>
#include <cilantro/io.hpp>
#include <iostream>

cilantro::NeighborSet<float> foobar(const Eigen::Matrix3Xf& points) {
  cilantro::KDTree3f tree(points);
  return tree.radiusSearch(Eigen::Vector3f(0.1, 0.1, 0.4), 1.001);
}

struct Foo {
  Foo(const Eigen::Matrix3Xf& points) : tree(points) {}

  cilantro::NeighborSet<float> bar() {
    return tree.radiusSearch(Eigen::Vector3f(0.1, 0.1, 0.4), 1.001);
  }
  
  cilantro::KDTree3f tree;
};

int main(int argc, char** argv) {
  Eigen::Matrix3Xf points(3, 8);
  points << 0, 1, 0, 0, 0, 1, 1, 1,
            0, 0, 1, 0, 1, 0, 1, 1,
            0, 0, 0, 1, 1, 1, 0, 1;

  cilantro::NeighborSet<float> nn;
  { // Causes invalid read
    Foo foo(points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5));
    nn = foo.bar();
  }
  { // no issues
    Eigen::Matrix3Xf centered =
        points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5);
    Foo foo(centered);
    nn = foo.bar();
  }
  { // no issues
    nn = foobar(points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5));
  }
  return 0;
}

Instantiating Foo with a temporary as in the first call leads to invalid reads that trace back to evalMetric in nanoflann. The invalid read does not show up in the second instantiation of Foo. Similarly the call to foobar does not cause an issue.

Seems more related to third party libs, but it would be great if we could catch these subtleties somehow. Lost quite some time over this one.

@wannesvanloock
Copy link
Contributor Author

I think I understand what might be the issue. cilantro::KDTreeDataAdaptors takes an Eigen::Map and remaps the memory to a new matrix. Passing it as a temporary to the constructor means the temporary will be destroyed after the call to the constructor. Hence, the Eigen::Map is now pointing to unallocated memory. Tricky...

@kzampog
Copy link
Owner

kzampog commented Jul 13, 2019

Yes, KDTree needs the point set to be 'alive'. Maybe we could just disable building from temporaries or use some Eigen::Ref-like mechanism to keep them alive? Any suggestions are welcome! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants