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.
