Re: Problem with pg_atomic_compare_exchange_u64 at 32-bit platforms

On Wed, May 20, 2020 at 10:59:44AM +0300, Konstantin Knizhnik wrote:
> 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;

PostgreSQL does things like that, so the assertions aren't frivolous.

> 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?

Most functions don't have such assertions.  That doesn't make it wrong for
this function to have them.

> 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?

I expect at least some platforms need strict alignment, though I haven't tried
to prove it.

> >>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).

Given https://postgr.es/m/flat/20150108204635.GK6299%40alap3.anarazel.de was
necessary, that is plausible.  Again, if you can provide a test case that you
have confirmed reproduces it, that will remove all doubt.  You refer to a "we"
that has access to a system that reproduces it.