Merge pull request #270 from neobrain/bitfield_fixes

Bitfield fixes

2 files changed, 58 insertions, 3 deletions

diff --git a/Source/Core/Common/BitField.h b/Source/Core/Common/BitField.h
index 40e748229e..f7a0a0175c 100644
--- a/Source/Core/Common/BitField.h
+++ b/Source/Core/Common/BitField.h
@@ -36,6 +36,8 @@
 #include <limits>
 #include <type_traits>
 
+#include "Common.h"
+
 /*
  * Abstract bitfield class
  *
@@ -92,13 +94,23 @@
  *
  * Caveats:
  *
+ * 1)
  * BitField provides automatic casting from and to the storage type where
  * appropriate. However, when using non-typesafe functions like printf, an
  * explicit cast must be performed on the BitField object to make sure it gets
  * passed correctly, e.g.:
  * printf("Value: %d", (s32)some_register.some_signed_fields);
  *
+ * 2)
+ * Not really a caveat, but potentially irritating: This class is used in some
+ * packed structures that do not guarantee proper alignment. Therefore we have
+ * to use #pragma pack here not to pack the members of the class, but instead
+ * to break GCC's assumption that the members of the class are aligned on
+ * sizeof(StorageType).
+ * TODO(neobrain): Confirm that this is a proper fix and not just masking
+ * symptoms.
  */
+#pragma pack(1)
 template<std::size_t position, std::size_t bits, typename T>
 struct BitField
 {
@@ -113,13 +125,13 @@ public:
 	// so that we can use this within unions
 	BitField() = default;
 
-	BitField& operator=(T val)
+	__forceinline BitField& operator=(T val)
 	{
 		storage = (storage & ~GetMask()) | ((val << position) & GetMask());
 		return *this;
 	}
 
-	operator T() const
+	__forceinline operator T() const
 	{
 		if (std::numeric_limits<T>::is_signed)
 		{
@@ -144,7 +156,7 @@ private:
 	// Unsigned version of StorageType
 	typedef typename std::make_unsigned<StorageType>::type StorageTypeU;
 
-	StorageType GetMask() const
+	__forceinline StorageType GetMask() const
 	{
 		return ((~(StorageTypeU)0) >> (8*sizeof(T) - bits)) << position;
 	}
@@ -158,3 +170,4 @@ private:
 	static_assert(bits <= 8 * sizeof(T), "Invalid number of bits");
 	static_assert(bits > 0, "Invalid number of bits");
 };
+#pragma pack()
diff --git a/Source/UnitTests/Common/BitFieldTest.cpp b/Source/UnitTests/Common/BitFieldTest.cpp
index 16efd29d19..800bb84360 100644
--- a/Source/UnitTests/Common/BitFieldTest.cpp
+++ b/Source/UnitTests/Common/BitFieldTest.cpp
@@ -125,3 +125,45 @@ TEST(BitField, Assignment)
 		EXPECT_EQ(object.regular_field_unsigned, object.at_dword_boundary);
 	}
 }
+
+// Test class behavior on oddly aligned structures.
+TEST(BitField, Alignment)
+{
+	#pragma pack(1)
+	struct OddlyAlignedTestStruct
+	{
+		u8 padding;
+		TestUnion obj;
+	};
+	#pragma pack()
+
+	GC_ALIGNED16(OddlyAlignedTestStruct test_struct);
+	TestUnion& object = test_struct.obj;
+	static_assert(alignof(test_struct.obj.signed_1bit) == 1, "Incorrect variable alignment");
+
+	for (u64 val : table)
+	{
+		// Assignments with fixed values
+		object.full_u64 = val;
+		EXPECT_EQ(val, object.full_u64);
+
+		object.full_s64 = (s64)val;
+		EXPECT_EQ((s64)val, object.full_u64);
+
+		object.regular_field_unsigned = val;
+		EXPECT_EQ(val & 0x7, object.regular_field_unsigned);
+
+		object.at_dword_boundary = val;
+		EXPECT_EQ(((s64)(val << 60)) >> 60, object.at_dword_boundary);
+
+		object.signed_1bit = val;
+		EXPECT_EQ((val & 1) ? -1 : 0, object.signed_1bit);
+
+		object.regular_field_signed = val;
+		EXPECT_EQ(((s64)(object.hex << 61)) >> 61, object.regular_field_signed);
+
+		// Assignment from other BitField
+		object.at_dword_boundary = object.regular_field_unsigned;
+		EXPECT_EQ(object.regular_field_unsigned, object.at_dword_boundary);
+	}
+}