libmesh
libmesh copied to clipboard
libMesh
Recent gcc won't let us use packed ranges with TypeVector, Point
Declaring a std::map<processor_id_type , std::vector<TypeVector<Real>>> vals and then trying to perform a parallel set_union(vals) is enough to demonstrate the problem. set_union correctly detects that we need to turn the map::value_type into a packed range, our Packing overload for std::vector correctly detects IsFixed<TypeVector>::value and correctly concludes that it's safe to pack each individual TypeVector with a memcpy ... and then gcc screams a warning at us about copying an object of non-trivial type.
TypeVector is indeed not TriviallyCopyable, and the C++ standard correctly says that should be undefined behavior for std::memcpy, so I tried giving TypeVector a default copy constructor and default destructor (both of which are trivial), and I verified that it then passed is_trivially_copyable static assertions ...
And that's not enough. GCC doesn't just warn about !is_trivially_copyable, it warns about !is_trivial, and they won't fix that, and we can't satisfy that (because we have our default constructors zero out their data). I don't want to just pragma away the warning here either, because when it's not a false positive it's actually super useful.
My plan B was going to be to manually add Packing overloads for such classes, but as I type this out I'm thinking instead that we might just insist on more precise warnings manually: in TIMPI packing.h we'd cast our T* to char* to avoid gcc's warning but we'd instead add an is_trivially_copyable static assertion to give us a saner warning, and then in libMesh classes we'd be able to get away with just satisfying is_trivially_copyable.
Pinging @bwspenc because he caught the problem today and because I expect him to remind me about this issue next week when I have time to work on it.
Okay - the problem is now fixed for TypeVector and Point, but it's not fixed in general: the C++ standard doesn't mandate that is_trivially_copyable<tuple<SomethingTriviallyCopyable>>::value == true, and that's too important a case for future use, so we'll need our own metafunction instead. We'll have ActuallyTriviallyCopyable default to false, specialize it to be true whenever is_trivially_copyable is, but then add specializations to our own metafunction for whichever containers might give us false negatives from the std:: metafunction.
This still isn't as good as I'd like it to be, because we have no way to automatically add a specialization for every ThirdPartyClass that would have been is_trivially_copyable if not for e.g. a tuple member variable, but at least cases like that will be able to add their own specializations manually.
the C++ standard doesn't mandate that
is_trivially_copyable<tuple<SomethingTriviallyCopyable>>::value == true
Isn't there some good reason for this? It sounds like you can't guarantee this in general, so I'd be wary about writing your own ActuallyTriviallyCopyable...
For tuple, things are a lot more complicated. Many tuple implementations are based on having a storage buffer of the right size/alignment, and using placement new to construct the individual members within that buffer.
Perhaps I should give it a different name? "MemCopyable"? But this just seems like a question of vocabulary - that link goes on to say, "Even if it knew that they were all trivially copyable and copied them via memcpy". Unless I'm reading it wrong, it's saying that some tuple implementations might not be able to get away with an = default copy constructor, but it's not saying that the same bytes in a different location won't give you a valid copy of a tuple of trivially copyable contents, and that's all we're concerned with asserting.
The behavior of a program that adds specializations for is_trivially_copyable or is_trivially_copyable_v (since C++17) is undefined.
https://en.cppreference.com/w/cpp/types/is_trivially_copyable
I guess that's why you are wanting to specialize our own ActuallyTriviallyCopyable class instead?
Exactly. I really do want to scream if someone ends up trying to memcpy() into an object for which just copying bytes is not a valid way to copy an object, but it turns out there's nothing in the C++ standard that I can test to determine that.
Huh. But now that I think about it... I seized on "ActuallyTriviallyCopyable" when it turned out that !is_trivially_copyable can be a lie, but maybe I should just be checking for a TIMPI::StandardType instead? For any ActuallyTriviallyCopyable type we ought to be able to define a StandardType overload, and if we want to be able to communicate those types via direct fixed-size communication and not just via packed buffers then we need to define that overload, so maybe I should just double-check that the overload is defined rather than creating a cut-down duplicate of it?
And then if I do that, I just need to get rid of the assert altogether. We're already using StandardType::is_fixed_type as a metafunction argument to select the overload that uses memcpy.
Lol at this line in https://en.cppreference.com/w/cpp/types/is_trivially_copyable
Objects of trivially-copyable types that are not potentially-overlapping subobjects are the only C++ objects that may be safely copied with std::memcpy
Yeah, I bet it was annoyances like this that led me to just not care about us doing things like specializing std::hash. If your language can't figure out that pair<int,int> is trivially copyable... anyway, I'm still planning to fix the hash situation, I just wish "planning to fix obviously wrong design" was a trait the C++ Standard had too. :fire: