Bizarre malloc error in CalculateCurvature

[geoffder]

I've finally gotten to pulling down the latest master and started updating the C bindings and have hit a bit of a snag. Everything compiles and tests fine for me with CUDA enabled, leading to mallocManaged using cudaMallocManaged instead of malloc. So malloc is being tripped up by size parameters that are doing fine in the CUDA branch. Any idea what could be going wrong? Won't have more time to try and figure it out for a bit, so dumping here to see if it makes sense to you guys.

Build Error:

In file included from /home/geoff/git/manifold/src/utilities/include/sparse.h:22,
                 from /home/geoff/git/manifold/src/collider/include/collider.h:17,
                 from /home/geoff/git/manifold/src/manifold/src/impl.h:18,
                 from /home/geoff/git/manifold/src/manifold/src/properties.cpp:21:
In static member function ‘static void manifold::ManagedVec<T>::mallocManaged(T**, size_t) [with T = glm::vec<3, int, glm::packed_highp>]’,
    inlined from ‘void manifold::ManagedVec<T>::shrink_to_fit() [with T = glm::vec<3, int, glm::packed_highp>]’ at /home/geoff/git/manifold/src/utilities/include/vec_dh.h:169:20,
    inlined from ‘void manifold::VecDH<T>::resize(int, T) [with T = glm::vec<3, int, glm::packed_highp>]’ at /home/geoff/git/manifold/src/utilities/include/vec_dh.h:349:36,
    inlined from ‘void manifold::Manifold::Impl::CalculateCurvature(int, int)’ at /home/geoff/git/manifold/src/manifold/src/properties.cpp:355:39:
/home/geoff/git/manifold/src/utilities/include/vec_dh.h:260:42: error: argument 1 range [18446744065119617032, 18446744073709551604] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
  260 |       *ptr = reinterpret_cast<T *>(malloc(bytes));
      |                                    ~~~~~~^~~~~~~
In file included from /usr/include/c++/13.1.1/cstdlib:79,
                 from /usr/include/c++/13.1.1/ext/string_conversions.h:43,
                 from /usr/include/c++/13.1.1/bits/basic_string.h:4110,
                 from /usr/include/c++/13.1.1/string:54,
                 from /usr/include/c++/13.1.1/bits/locale_classes.h:40,
                 from /usr/include/c++/13.1.1/bits/ios_base.h:41,
                 from /usr/include/c++/13.1.1/streambuf:43,
                 from /usr/include/c++/13.1.1/bits/streambuf_iterator.h:35,
                 from /usr/include/c++/13.1.1/iterator:66,
                 from /home/geoff/git/manifold/src/third_party/thrust/thrust/iterator/iterator_traits.h:36,
                 from /home/geoff/git/manifold/src/third_party/thrust/thrust/count.h:26,
                 from /home/geoff/git/manifold/src/manifold/src/properties.cpp:15:
/usr/include/stdlib.h: In member function ‘void manifold::Manifold::Impl::CalculateCurvature(int, int)’:
/usr/include/stdlib.h:553:14: note: in a call to allocation function ‘void* malloc(size_t)’ declared here
  553 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
      |              ^~~~~~

Machine:

OS: EndeavourOS Linux (Arch based)
Kernel: 6.3.7-zen1-1-ze

[elalish]

I've never seen an error like that; how can it know an allocation is too big at compile time? @pca006132 any thoughts, since you did the ManagedVec work?

[geoffder]

For a bit more context now that I;m poking it again, it is this line as mentioned by the error message. Specifically, it doesn't like that NumTri() is informing the number of bytes that should be allocated. Giving an int literal compiles fine.

  if (meshRelation_.triProperties.size() == 0) {
    // int n = NumTri(); // error
    int n = 1000000000;  // compiles
    meshRelation_.triProperties.resize(n);
  }

[geoffder]

Bizarrely, this compiles. Looking at resize and reserve in VecDH, I'm not sure exactly what this avoids, since resize is supposed to be doing a reserve anyway...

  if (meshRelation_.triProperties.size() == 0) {
    int n = NumTri();
    meshRelation_.triProperties.reserve(n);
    meshRelation_.triProperties.resize(n);
  }

[elalish]

That's bizarre - what compiler is this? I feel like I have lines all over the code base that do that; it seems strange it would only complain about that one.

[geoffder]

what compiler is this?

This was built with the default system compiler (gcc).

❯ /usr/bin/c++ --version
c++ (GCC) 13.1.1 20230429

It definitely is weird, resize is used indirectly with functions of NumVerts() etc all over the place, as the constructors of VecDH call it. I don't know why this particular line is tripping it up, and why adding a reserve before avoids it.

[geoffder]

Given that adding a line that should be redundant avoids the error, I got suspicious that it is an optimization issue, and I think that I'm right.

set(MANIFOLD_FLAGS -O0)

Turning off optimizations in Manifold by setting -O0 in CMakeLists.txt compiles for me. Setting -O1 hits the same error.

[geoffder]

Modifying ManagedVec::shrink_to_fit like so:

  void shrink_to_fit() {
    T *newBuffer = nullptr;
    if (size_ > 0) {
      // mallocManaged(&newBuffer, size_ * sizeof(T));      
      int sz = size_ * sizeof(T);
      mallocManaged(&newBuffer, sz);
      prefetch(newBuffer, size_ * sizeof(T), onHost);
      uninitialized_copy(autoPolicy(size_), ptr_, ptr_ + size_, newBuffer);
    }
    freeManaged(ptr_);
    ptr_ = newBuffer;
    capacity_ = size_;
  }

avoids the error for me (without the pre-emptive reserve line mentioned above).

[geoffder]

This issue I found in my travels may be related. Maybe breaking out the size calculation to another line defeats the inlining somehow?

[elalish]

That looks like a perfectly reasonable change, even if it is a GCC bug workaround. The wisdom I always heard was if you think it's a compiler bug, it's really your own bug, but here I'm tempted to disagree.

[geoffder]

Ok, I'll make a PR with this fix then.

[pca006132]

I agree this is very weird... Perhaps gcc somehow thinks that NumTri can return something negative?

[geoffder]

I agree this is very weird... Perhaps gcc somehow thinks that NumTri can return something negative?

I did experiment with that, but casting to unsigned didn't change anything in this case (though I saw examples online where that fixed peoples malloc problems).

[pentacular]

Just from curiosity, what is the point of reserve(n) followed by resize(n)?

[geoffder]

@pentacular There isn't, but while investigating the bug, I added the reserve in order to see if the error would come up on the earlier line (when I thought it might have been reserve in resize that was the culprit, which was not correct). Instead of breaking on reserve(n) it actually compiled successfully, which gave me the idea that it was an interaction between some inlining optimization and the malloc warning/error.

This is basically repeating what I described in my comments above though.