Fix offset calculation for ivar bitfields.

In some cases, we were allowing ivars declared as bitfields to overlap
other ivars, which caused memory corruption.

This showed up in OOlite.

Author: davidchisnall

Test/CMakeLists.txt

@@ -13,6 +13,7 @@ endif ()
 set(TESTS
 	alias.m
 	alignTest.m
+	bitfield.m
 	AllocatePair.m
 	AssociatedObject.m
 	AssociatedObject2.m

Test/IVarSuperclassOverlap.m

@@ -89,9 +89,18 @@ int main(int argc, char *argv[])
 	assert(ivar_getOffset(c3) == baseSmallOffset + 4);
 	assert(ivar_getOffset(b1) == baseSmallOffset + 2);
 	assert(ivar_getOffset(b2) == baseSmallOffset + 2);
-	assert(ivar_getOffset(b3) == baseSmallOffset + 3);
-	assert(ivar_getOffset(b4) == baseSmallOffset + 3);
-	assert(ivar_getOffset(notBitfield) == baseSmallOffset + 4);
+	// These could be tighter, but the way that clang currently emits ivars for
+	// bitfields is a bit odd.  Unfortunately, it sometimes adds a small
+	// displacement so that it can load individual words, but the metadata
+	// doesn't tell us anything about them.  Fixing this would require using an
+	// extra metadata flag or similar to indicate that the size of the ivar is
+	// not the storage size.
+	assert((ivar_getOffset(b3) == baseSmallOffset + 3) ||
+	       (ivar_getOffset(b3) == baseSmallOffset + 4));
+	assert((ivar_getOffset(b4) == baseSmallOffset + 3) ||
+	       (ivar_getOffset(b4) == baseSmallOffset + 4));
+	assert((ivar_getOffset(notBitfield) == baseSmallOffset + 4) ||
+	       (ivar_getOffset(notBitfield) == baseSmallOffset + 6));
 #endif
 
 

Test/bitfield.m

@@ -0,0 +1,57 @@
+#include "stdio.h"
+#include "Test.h"
+#include "objc/runtime.h"
+#include "objc/encoding.h"
+
+@interface Bitfield : Test
+{
+	unsigned short first;
+	unsigned  isShip: 1,
+	          isStation: 1;
+	unsigned y;
+}
+
+@end
+@implementation Bitfield
+- (BOOL)isShip { return isShip; }
+- (BOOL)isStation { return isStation; }
+- (void)setShip: (BOOL)aValue
+{
+	isShip = aValue;
+}
+- (void)setStation: (BOOL)aValue
+{
+	isStation = aValue;
+}
+- (void)setY: (int)anInt
+{
+	y = anInt;
+}
+@end
+
+static size_t offset(const char *ivar)
+{
+	return ivar_getOffset(class_getInstanceVariable([Bitfield class], ivar));
+}
+
+static size_t size(const char *ivar)
+{
+	return objc_sizeof_type(ivar_getTypeEncoding(class_getInstanceVariable([Bitfield class], ivar)));
+}
+
+int main(void)
+{
+	Bitfield *bf = [Bitfield new];
+	assert(![bf isShip]);
+	assert(![bf isStation]);
+	[bf setShip: YES];
+	assert([bf isShip]);
+	assert(![bf isStation]);
+	[bf setStation: YES];
+	[bf setY: 0];
+	assert([bf isShip]);
+	assert([bf isStation]);
+	assert(offset("isShip") >= offset("first") + size("first"));
+	assert(offset("isShip") == offset("isStation"));
+	assert(offset("y") >= offset("isStation") + size("isStation"));
+}

ivar.c

@@ -85,25 +85,25 @@ PRIVATE void objc_compute_ivar_offsets(Class class)
 				// that contains them.  If we are in a bitfield, then we need
 				// to make sure that we don't add any displacement from the
 				// previous value.
-				if (*ivar->offset < last_offset + last_size)
+				if ((i != 0) && (*ivar->offset == last_offset))
 				{
-					*ivar->offset = last_computed_offset + (*ivar->offset - last_offset);
-					ivar_size = 0;
+					*ivar->offset = last_computed_offset;
 					continue;
 				}
 				last_offset = *ivar->offset;
 				*ivar->offset = next_ivar;
-				last_computed_offset = *ivar->offset;
 				next_ivar += ivar_size;
 				last_size = ivar->size;
 				size_t align = ivarGetAlign(ivar);
+				// If the alignment is insufficient, round it up.
 				if ((*ivar->offset + refcount_size) % align != 0)
 				{
 					long padding = align - ((*ivar->offset + refcount_size) % align); 
 					*ivar->offset += padding;
 					class->instance_size += padding;
 					next_ivar += padding;
 				}
+				last_computed_offset = *ivar->offset;
 				assert((*ivar->offset + sizeof(uintptr_t)) % ivarGetAlign(ivar) == 0);
 				class->instance_size += ivar_size;
 			}