On 20.05.2020 10:36, Noah Misch wrote:
On Wed, May 20, 2020 at 10:23:37AM +0300, Konstantin Knizhnik wrote:
On 20.05.2020 06:05, Noah Misch wrote:
On Tue, May 19, 2020 at 04:07:29PM +0300, Konstantin Knizhnik wrote:
Definition of pg_atomic_compare_exchange_u64 requires alignment of expected
pointer on 8-byte boundary.
pg_atomic_compare_exchange_u64(volatile pg_atomic_uint64 *ptr,
uint64 *expected, uint64 newval)
{
#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
AssertPointerAlignment(ptr, 8);
AssertPointerAlignment(expected, 8);
#endif
I wonder if there are platforms where such restriction is actually needed.
In general, sparc Linux does SIGBUS on unaligned access. Other platforms
function but suffer performance penalties.
Well, if platform enforces strict alignment, then addressed value should be
properly aligned in any case, shouldn't it?
No. One can always cast a char* to a uint64* and get a misaligned read when
dereferencing the resulting pointer.
Yes, certainly we can "fool" compiler using type casts:
char buf[8];
*(int64_t*)buf = 1;
But I am speaking about normal (safe) access to variables:
long long x;
In this case "x" compiler enforces proper alignment of "x" for the
target platform.
We are not adding AssertPointerAlignment to any function which has
pointer arguments, aren' we?
I understand we do we require struct alignment pointer to atomic
variables even at the platforms which do not require it
(as Andreas explained, if value cross cacheline, it will cause
significant slowdown).
But my question was whether we need string alignment of expected value?
void f() {
int x;
long long y;
printf("%p\n", &y);
}
then its address must not be aligned on 8 at 32-bit platform.
This is why "expected" in test_atomic_uint64 may not be aligned on 8-byte
boundary and we can get assertion failure.
Can you construct a patch that adds some automatic variables to a regress.c
function and causes an assertion inside pg_atomic_compare_exchange_u64() to
fail on some machine you have? I don't think win32 behaves as you say. If
you can make a test actually fail using the technique you describe, that would
remove all doubt.
I do not have access to Win32.
But I think that if you just add some 4-byte variable before "expected"
definition, then you will get this assertion failure (proposed patch is
attached).
Please notice that PG_HAVE_ATOMIC_U64_SIMULATION should not be defined
and Postgres is build with --enable-cassert and CLAGS=-O0
Also please notice that my report is not caused just by hypothetical
problem which I found out looking at Postgres code.
We actually get this assertion failure in pg_atomic_compare_exchange_u64
at Win32 (not in regress.c).
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 4e32df2c31d..1b2e74574c3 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -753,8 +753,8 @@ static void
test_atomic_uint64(void)
{
pg_atomic_uint64 var;
- uint64 expected;
int i;
+ uint64 expected;
pg_atomic_init_u64(&var, 0);
EXPECT_EQ_U64(pg_atomic_read_u64(&var), 0);
