Re: Problem with pg_atomic_compare_exchange_u64 at 32-bit platforms

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.

> >>And if so, looks like our ./src/test/regress/regress.c is working only
> >>occasionally:
> >>
> >>static void
> >>test_atomic_uint64(void)
> >>{
> >>     pg_atomic_uint64 var;
> >>     uint64        expected;
> >>     ...
> >>         if (!pg_atomic_compare_exchange_u64(&var, &expected, 1))
> >>
> >>because there is no warranty that "expected" variable will be aligned on
> >>stack at 8 byte boundary (at least at Win32).
> >src/tools/msvc sets ALIGNOF_LONG_LONG_INT=8, so it believes that win32 does
> >guarantee 8-byte alignment of both automatic variables. Is it wrong?
>
> Yes, by default "long long" and "double" types are aligned on 8-byte
> boundary at 32-bit Windows (but not at 32-bit Linux).
> Bu it is only about alignment of fields inside struct.
> So if you define structure:
>
> typedef struct {
>      int x;
>      long long y;
> } foo;
>
> then sizeof(foo) will be really 16 at Win32.'
> But Win32 doesn't enforce alignment of stack frames on 8-byte boundary.
> It means that if you define local variable "y":
>
> 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.