pioneerLib.dir/src/CollMesh.cpp does not compile on i686 arch

[sagitter]

Observed behaviour

pioneerLib.dir/src/CollMesh.cpp compilation is failed on Fedora 30 i686 architecture with the following error:

[ 12%] Building CXX object CMakeFiles/pioneerLib.dir/src/CollMesh.cpp.o
/usr/bin/c++   -I/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/build -I/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f -I/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/contrib -I/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src -I/usr/include/assimp -I/usr/include/freetype2 -I/usr/include/SDL2 -I/usr/include/sigc++-2.0 -I/usr/lib/sigc++-2.0/include -I/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/contrib/lua  -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fpermissive -DNDEBUG   -fdiagnostics-color -std=gnu++11 -o CMakeFiles/pioneerLib.dir/src/CollMesh.cpp.o -c /builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/CollMesh.cpp
make[2]: Leaving directory '/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/build'
In file included from /builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/CollMesh.cpp:5:
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h:26:37: error: static assertion failed: Int64 is sized differently on this platform and will not serialize properly.
   26 |  static_assert((sizeof(Uint64) == 8 && alignof(Uint64) == 8), "Int64 is sized differently on this platform and will not serialize properly.");
      |                ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h:29:40: error: static assertion failed: Vector2d is padded differently on this platform and will not serialize properly.
   29 |  static_assert((sizeof(vector2d) == 16 && alignof(vector2d) == 8), "Vector2d is padded differently on this platform and will not serialize properly.");
      |                ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h:31:40: error: static assertion failed: Vector3d is padded differently on this platform and will not serialize properly.
   31 |  static_assert((sizeof(vector3d) == 24 && alignof(vector3d) == 8), "Vector3d is padded differently on this platform and will not serialize properly.");
      |                ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h:33:36: error: static assertion failed: Aabb is padded differently on this platform and will not serialize properly.
   33 |  static_assert((sizeof(Aabb) == 56 && alignof(Aabb) == 8), "Aabb is padded differently on this platform and will not serialize properly.");
      |                ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h: In member function 'ByteRange Serializer::Reader::Blob()':
/builddir/build/BUILD/pioneer-e85a0cf4ca8e710d926f6c1581c3d9db242f361f/src/scenegraph/Serializer.h:184:12: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare]
  184 |    if (len > (m_data.end - m_at))
      |        ~~~~^~~~~~~~~~~~~~~~~~~~~
make[2]: *** [CMakeFiles/pioneerLib.dir/build.make:183: CMakeFiles/pioneerLib.dir/src/CollMesh.cpp.o] Error 1

Expected behaviour

Correctly compiled.

Steps to reproduce

Try to compile on Fedora with

• cmake-3.14.5
• gcc-9.2.1
• Full build log: https://kojipkgs.fedoraproject.org//work/tasks/89/38150089/build.log
• Full list of dependencies: https://kojipkgs.fedoraproject.org//work/tasks/89/38150089/root.log
• Pioneer commit e85a0cf

[Web-eWorks]

I'm going to take a stab in the dark and guess that on i686 the alignof(uint64_t) is 4 instead of the 8 I would expect. Same with doubles, which would be the issue there. Can you try changing the affected lines to read alignof(<type>) <= 8? I think that would resolve the issue, at least temporarily.

[sagitter]

This patch is working:

--- a/src/scenegraph/Serializer.orig.h	2019-10-09 13:49:04.000000000 +0200
+++ b/src/scenegraph/Serializer.h	2019-10-09 18:47:05.238671730 +0200
@@ -23,14 +23,14 @@
 // where possible, prefer serializing state information via JSON instead.
 namespace Serializer {
 	static_assert((sizeof(Uint32) == 4 && alignof(Uint32) == 4), "Int32 is sized differently on this platform and will not serialize properly.");
-	static_assert((sizeof(Uint64) == 8 && alignof(Uint64) == 8), "Int64 is sized differently on this platform and will not serialize properly.");
+	static_assert((sizeof(Uint64) == 8 && alignof(Uint64) <= 8), "Int64 is sized differently on this platform and will not serialize properly.");
 	static_assert((sizeof(Color) == 4 && alignof(Color) == 1), "Color is padded differently on this platform and will not serialize properly.");
 	static_assert((sizeof(vector2f) == 8 && alignof(vector2f) == 4), "Vector2f is padded differently on this platform and will not serialize properly.");
-	static_assert((sizeof(vector2d) == 16 && alignof(vector2d) == 8), "Vector2d is padded differently on this platform and will not serialize properly.");
+	static_assert((sizeof(vector2d) == 16 && alignof(vector2d) <= 8), "Vector2d is padded differently on this platform and will not serialize properly.");
 	static_assert((sizeof(vector3f) == 12 && alignof(vector3f) == 4), "Vector3f is padded differently on this platform and will not serialize properly.");
-	static_assert((sizeof(vector3d) == 24 && alignof(vector3d) == 8), "Vector3d is padded differently on this platform and will not serialize properly.");
+	static_assert((sizeof(vector3d) == 24 && alignof(vector3d) <= 8), "Vector3d is padded differently on this platform and will not serialize properly.");
 	static_assert((sizeof(Quaternionf) == 16 && alignof(Quaternionf) == 4), "Quaternionf is padded differently on this platform and will not serialize properly.");
-	static_assert((sizeof(Aabb) == 56 && alignof(Aabb) == 8), "Aabb is padded differently on this platform and will not serialize properly.");
+	static_assert((sizeof(Aabb) == 56 && alignof(Aabb) <= 8), "Aabb is padded differently on this platform and will not serialize properly.");
 
 	class Writer {
 	public:

[impaktor]

@Web-eWorks / @fluffyfreak is this a bug, and if so, what desired action should be taken, in the best of worlds?

Naturally, it would be nice to support as many platforms at possible, but not at the (unreasonable) cost of code complexity, or dev-time, in my opinion.

[fluffyfreak]

It is kind of a bug, it's means that the serialisation will be platform dependent and save games might not be portable across platforms/architectures.

Since this is just the alignment it might be ok, as I believe that we (de/)serialise data structures as individual values (?) rather than directly as binary blobs. However I can't be 100% certain of that, there's so much code I don't know it all anymore.

[impaktor]

OK, thanks for explenation, then this sounds like we're keeping this open, for now.

[Web-eWorks]

This is a left-over canary from my attempt to speed up serialization of model and collision data. We don't actually write any of these structures in their whole form anymore, so this isn't quite necessary. It is somewhat useful to catch platforms where alignment is very non-standard, but ultimately these checks can be removed.

[Web-eWorks]

I'm experimenting with implementing a Bounding Interval Hierarchy tree (AKA a loose kd-tree) instead of our current BVH tree for triangle collision acceleration, so I'll clean up the serializer code at the end of that PR.

[fluffyfreak]

Is this caused by the removal of the #pragma pack 4 in vector3.h?
https://github.com/pioneerspacesim/pioneer/blob/master/src/vector3.h#L13

[Web-eWorks]

@fluffyfreak no, it's caused by double being 4-byte aligned on 32-bit systems. It's not a big problem - in fact it's just an overzealous assumption.