RecursiveEdgeSwap invalid memory access about manifold

[deadman2000]

I have similar problem

Test for reproduce:

TEST(Manifold, Substract) {
  Mesh firstMesh;
  firstMesh.vertPos = {
      {0, 0, 0},           {1540, 0, 0},      {1540, 70, 0},
      {0, 70, 0},          {0, 0, -278.282},  {1540, 70, -278.282},
      {1540, 0, -278.282}, {0, 70, -278.282}};
  firstMesh.triVerts = {
      {0, 1, 2}, {2, 3, 0}, {4, 5, 6}, {5, 4, 7}, {6, 2, 1}, {6, 5, 2},
      {5, 3, 2}, {5, 7, 3}, {7, 0, 3}, {7, 4, 0}, {4, 1, 0}, {4, 6, 1},
  };

  Mesh secondMesh;
  secondMesh.vertPos = {
      {2.04636e-12, 70, 50000},       {2.04636e-12, -1.27898e-13, 50000},
      {1470, -1.27898e-13, 50000},    {1540, 70, 50000},
      {2.04636e-12, 70, -28.2818},    {1470, -1.27898e-13, 0},
      {2.04636e-12, -1.27898e-13, 0}, {1540, 70, -28.2818}};
  secondMesh.triVerts = {{0, 1, 2}, {2, 3, 0}, {4, 5, 6}, {5, 4, 7},
                         {6, 2, 1}, {6, 5, 2}, {5, 3, 2}, {5, 7, 3},
                         {7, 0, 3}, {7, 4, 0}, {4, 1, 0}, {4, 6, 1}};

  Manifold first(firstMesh);
  Manifold second(secondMesh);

  first -= second;
  first.GetMesh();
}

[pca006132]

Test case from #141 (comment)

This is probably related to precision.

TEST(Boolean, Close) {
  Manifold a = Manifold::Sphere(10, 256);
  Manifold result = a;
  for (int i = 0; i < 10; i++) {
    std::cout << i << std::endl;
    result ^= a.Translate({a.Precision()/10 * i, 0.0, 0.0});
    EXPECT_TRUE(result.IsManifold());
    EXPECT_TRUE(result.MatchesTriNormals());
  }
}

For circularSegments = 128, it triggered this issue. For circularSegments = 256, it shows triangulation failed without triggering this, but I guess the calculation is wrong anyway.

[elalish]

Interesting; I tried your test with valgrind (which is what I usually use for this kind of thing) and my usual GCC compiler, but it found no problems. I've never used -fsanitize, which I suppose shows I'm still a C++ noob in some ways, and it doesn't want to compile for me. Should we add that to our CI tests?

[pca006132]

I think you may need to install libsan according to this SO thread. I use sanitizers because it is much faster than valgrind, but I am not sure if it can catch more bugs than valgrind.

Yes we can compile all tests on our CI with address sanitizer enabled.

I tested with both clang++ 13.0.0 and g++ 10.3.0, so this should not be dependent on certain version of the compiler.

[elalish]

I found this SO answer, which helped (and also made me wonder if our CMakeLists is more complex than it needs to be?). However, I'm still not seeing any errors on your test. I'm on GCC 9.3.0. Might be best to go ahead and add this as a test so we can all see it fail the CI.

[pca006132]

Interesting. I can try to add this to the tests and reproduce this in the CI. Btw are you compiling a debug version or release version? Can you show me the full cmake command you used?

Regarding the CMakeLists, yes I think it is a bit too complicated and there are a lot of repeated code there. I'm not familiar with cmake so not sure what is the best way to handle that.

[elalish]

I'm just using VSCode's build button as usual, which does this: /usr/local/bin/cmake --build /home/emmett/code/manifold/build --config Release --target all -j 14 -- (I pasted it into a new test so that it's part of the main build).

I'm no CMake expert either; I think you may have written more of it than me at this point. They're always adding features in new versions that make things simpler. Anyway, just something to keep in mind.

[pca006132]

OK It turns out that I was using ed272b0 when trying to minimize the test case, and the behavior with the current master is a bit non-deterministic. The error can still be reproduced there, but we need to randomly permute the indices and try for some time... which is not easy to debug. Instead ed272b0 can deterministically fail.

Here is the CI run that failed: pca006132#2

[elalish]

Interesting, any thoughts as to why the lazy boolean would introduce more non-determinism here? Seems like without reordering it should be as deterministic as it was before...

[pca006132]

No idea, perhaps we violated const-ness somewhere or the csg_tree implementation is incorrect somewhere?

[pca006132]

Thanks, your example seems a bit simpler to look at than mine :)

[elalish]

Ah yes, the joys of precision... I never did do a proper mathematical bounding exercise. Sounds like I need to A) fix the seg fault, and B) probably loosen the precision a bit. Hopefully that's enough 🤞