Segmentation fault with Eigen library and GMPrationals [Solv

[amandini]

Hello,

I am doing some work using the Eigen C++ template library, with large sparse matrices, say 200,000 x 200,000, over the mpq_class provided by the GNU Multiple Precision (GMP) library.

The first problem is that code which compiles without any warnings on the godbolt compiler, a small example of which is given at https://godbolt.org/z/j66jdK7n6, produces the following warnings when compiled on my gentoo system

[sam_]

I see the warning locally as well but not on godbolt. Not sure why yet.

What does -march=native expand to on your system? You can check by running resolve-march-native from app-misc/resolve-march-native. I do see it without but it may be useful to know nonetheless.

Could you give me a testcase which segfaults? Can look more then.

[amandini]

[bstaletic]

I'm not familiar with GMP, but that operator< looks very wrong - sometimes it tells you which of the two inputs is smaller and sometimes it does the opposite.

[GDH-gentoo]

I don't know why the Compiler Explorer doesn't output the warning, but it makes sense. The code in Eigen::internal::AmbiVector<>'s reallocateSparse() member function uses std::memcpy() to copy the value of an object whose type is the type denoted by the class template's _Scalar parameter. I believe that you can have undefined behaviour when using objects of nontrivially copyable types made like that... and GMP's mpq_class is definitely not trivially copyable, it's a class that has nontrivial default and copy constructors, a nontrivial copy assignment operator, and a nontrivial destructor.

I don't know in general if Eigen documents its expectations about the types that are passed as template arguments like this, examples seem to show only arithmetic types like double or at most trivially copyable classes like std::complex<float>. The segfault shown in the backtrace happens in the destructor of some mpq_class object, hard to say if it was properly constructed.
_________________

[amandini]

[amandini]

[GDH-gentoo]

[amandini]

[Hu]

The warning about memcpy is because classes with non-trivial copy constructors often (but not always) have invariants that the copy constructor will maintain, but that a straight memcpy will not. A simple example of that would be std::vector: the copy constructor is nontrivial because the copy gets its own distinct storage, with its own copy of the contained objects. Using memcpy to copy a vector would result in two vector instances that point to the same storage, and both will try to free that storage when destroyed. That free is undefined behavior and may (or may not) cause a crash due to heap corruption.

I would argue that until you can patch Eigen to build cleanly with -Werror=class-memaccess, it is not worth your time investigating the crash further. Eliminating the undefined behavior of copying the non-trivial class is worthwhile, and may (or may not) fix your crash as a side effect.

[GDH-gentoo]

[GDH-gentoo]

[amandini]

[amandini]

[amandini]

[GDH-gentoo]

Interesting, does the GDB backtrace look the same?

Eigen::SparseMatrix<mpq_class, Eigen::RowMajor, int>::insertBackUncompressed() --> mpq_class destructor --> mpq_clear() --> std::free() --> segmentation fault
_________________

[amandini]

[GDH-gentoo]

I see. A little different from your original program, but the segmentation fault still happens when executing Eigen::SparseMatrix<mpq_class, Eigen::RowMajor, int>::insertBackUncompressed()'s code.

[amandini]

[amandini]

Update
The Eigen developers suggested that for the matrices I use

[Hu]

That program may work, but I still think Eigen needs an improvement here. It should either not use std::memcpy to copy the type, or it should include a compile-time check that the type being copied is safe to use with std::memcpy. In my opinion, it should not be possible for a user to build a program with likely dangerous behavior (byte-copying a type with a non-trivial copy constructor) without specifically bypassing a safety measure that warns the user about the expected problem.

[GDH-gentoo]

Resolving issue #2873 should take care of that. They where careful in other places of the code, they have for example a smart_copy<>() function template —which they reference in the issue— that uses fast std::memcpy() when safe (determined for type _Scalar by evaluating Eigen::NumTraits<_Scalar>::RequireInitialization), and slower but general std::copy() otherwise.

Bit I'm not sure this particular code path was taken in the examples that segfaulted, though. What Eigen developers suggested to amandini was explicitly passing a 64-bit integer type as the _StorageIndex template parameter, instead of using the 32-bit one (on amd64) that it defaults to. Looks to me like a different underlying problem.
_________________

[Hu]

That is better, but if a user sets RequireInitialization=0 on a type that needs a non-trivial copy constructor, will that (a) compile and run properly, (b) compile with undefined behavior, or (c) fail to compile? My concern is that users not be able to trigger path (b) without being very clear what they are doing.

[GDH-gentoo]

Either a) or b), I suppose, depending on what gets instantiated, but this falls under the general C++ question: if a template has requirements on its type parameters, whose responsibility is making sure that arguments meet them? If they don't, can this be detected at compile time?

[Hu]

For the sake of minimizing user confusion and subsequent support questions, my preference is that the template do the best validation that it easily can, through a combination of type_traits checks and static_assert statements on those traits. For this case, I would suggest adding static_assert(std::is_trivially_copyable<_Scalar>::value, "_Scalar values must be trivially copyable to use memcpy"); adjacent to that std::memcpy. A block comment could elaborate on how the user can avoid that path for values that are known not to be trivially copyable, such as by setting RequireInitialization=1 in the Eigen::NumTraits specialization (assuming that suffices to redirect the logic to the std::copy path; I am inferring that from your prior post).

If the library requires C++20 or later, it could use requires checks instead of static_assert. Either way, mistakes give the user a clear compile-time error, independent of compiler warning options.