Fix alignment of pg_atomic_uint64 variables on some 32bit platforms.

anarazel · anarazel · commit db4ec2ffce35 · 2015-01-11T01:06:37.000+01:00
I failed to recognize that pg_atomic_uint64 wasn't guaranteed to be 8
byte aligned on some 32bit platforms - which it has to be on some
platforms to guarantee the desired atomicity and which we assert.

As this is all compiler specific code anyway we can just rely on
compiler specific tricks to enforce alignment.

I've been unable to find concrete documentation about the version that
introduce the sunpro alignment support, so that might need additional
guards.

I've verified that this works with gcc x86 32bit, but I don't have
access to any other 32bit environment.

Discussion: op.xpsjdkil0sbe7t@vld-kuci

Per report from Vladimir Koković.
diff --git a/src/include/port/atomics/arch-x86.h b/src/include/port/atomics/arch-x86.h
@@ -73,6 +73,7 @@ typedef struct pg_atomic_uint32
 #define PG_HAVE_ATOMIC_U64_SUPPORT
 typedef struct pg_atomic_uint64
 {
+	/* alignment guaranteed due to being on a 64bit platform */
 	volatile uint64 value;
 } pg_atomic_uint64;
 #endif
diff --git a/src/include/port/atomics/generic-acc.h b/src/include/port/atomics/generic-acc.h
@@ -40,6 +40,12 @@ typedef struct pg_atomic_uint32
 #define PG_HAVE_ATOMIC_U64_SUPPORT
 typedef struct pg_atomic_uint64
 {
+	/*
+	 * Alignment is guaranteed to be 64bit. Search for "Well-behaved
+	 * application restrictions" => "Data alignment and data sharing" on HP's
+	 * website. Unfortunately the URL doesn't seem to stable enough to
+	 * include.
+	 */
 	volatile uint64 value;
 } pg_atomic_uint64;
 
diff --git a/src/include/port/atomics/generic-gcc.h b/src/include/port/atomics/generic-gcc.h
@@ -98,7 +98,7 @@ typedef struct pg_atomic_uint32
 
 typedef struct pg_atomic_uint64
 {
-	volatile uint64 value;
+	volatile uint64 value __attribute__((aligned(8)));
 } pg_atomic_uint64;
 
 #endif /* defined(HAVE_GCC__ATOMIC_INT64_CAS) || defined(HAVE_GCC__SYNC_INT64_CAS) */
diff --git a/src/include/port/atomics/generic-msvc.h b/src/include/port/atomics/generic-msvc.h
@@ -41,7 +41,7 @@ typedef struct pg_atomic_uint32
 } pg_atomic_uint32;
 
 #define PG_HAVE_ATOMIC_U64_SUPPORT
-typedef struct pg_atomic_uint64
+typedef struct __declspec(align(8)) pg_atomic_uint64
 {
 	volatile uint64 value;
 } pg_atomic_uint64;
diff --git a/src/include/port/atomics/generic-sunpro.h b/src/include/port/atomics/generic-sunpro.h
@@ -55,7 +55,13 @@ typedef struct pg_atomic_uint32
 #define PG_HAVE_ATOMIC_U64_SUPPORT
 typedef struct pg_atomic_uint64
 {
-	volatile uint64 value;
+	/*
+	 * Syntax to enforce variable alignment should be supported by versions
+	 * supporting atomic.h, but it's hard to find accurate documentation. If
+	 * it proves to be a problem, we'll have to add more version checks for 64
+	 * bit support.
+	 */
+	volatile uint64 value __attribute__((__aligned__(8)));
 } pg_atomic_uint64;
 
 #endif /* HAVE_ATOMIC_H */
diff --git a/src/include/port/atomics/generic-xlc.h b/src/include/port/atomics/generic-xlc.h
@@ -32,7 +32,7 @@ typedef struct pg_atomic_uint32
 #define PG_HAVE_ATOMIC_U64_SUPPORT
 typedef struct pg_atomic_uint64
 {
-	volatile uint64 value;
+	volatile uint64 value __attribute__((__aligned__(8)));
 } pg_atomic_uint64;
 
 #endif /* __64BIT__ */

Original file line number	Diff line number	Diff line change
`@@ -73,6 +73,7 @@ typedef struct pg_atomic_uint32`
`73`	`73`	`#define PG_HAVE_ATOMIC_U64_SUPPORT`
`74`	`74`	`typedef struct pg_atomic_uint64`
`75`	`75`	`{`
	`76`	`+ /* alignment guaranteed due to being on a 64bit platform */`
`76`	`77`	`volatile uint64 value;`
`77`	`78`	`} pg_atomic_uint64;`
`78`	`79`	`#endif`
Original file line number	Diff line number	Diff line change
`@@ -98,7 +98,7 @@ typedef struct pg_atomic_uint32`
`98`	`98`
`99`	`99`	`typedef struct pg_atomic_uint64`
`100`	`100`	`{`
`101`		`- volatile uint64 value;`
	`101`	`+ volatile uint64 value __attribute__((aligned(8)));`
`102`	`102`	`} pg_atomic_uint64;`
`103`	`103`
`104`	`104`	`#endif /* defined(HAVE_GCC__ATOMIC_INT64_CAS) \|\| defined(HAVE_GCC__SYNC_INT64_CAS) */`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ typedef struct pg_atomic_uint32`
`41`	`41`	`} pg_atomic_uint32;`
`42`	`42`
`43`	`43`	`#define PG_HAVE_ATOMIC_U64_SUPPORT`
`44`		`-typedef struct pg_atomic_uint64`
	`44`	`+typedef struct __declspec(align(8)) pg_atomic_uint64`
`45`	`45`	`{`
`46`	`46`	`volatile uint64 value;`
`47`	`47`	`} pg_atomic_uint64;`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ typedef struct pg_atomic_uint32`
`32`	`32`	`#define PG_HAVE_ATOMIC_U64_SUPPORT`
`33`	`33`	`typedef struct pg_atomic_uint64`
`34`	`34`	`{`
`35`		`- volatile uint64 value;`
	`35`	`+ volatile uint64 value __attribute__((__aligned__(8)));`
`36`	`36`	`} pg_atomic_uint64;`
`37`	`37`
`38`	`38`	`#endif /* __64BIT__ */`