Re: Broken atomics code on PPC with FreeBSD 10.3

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jan 3, 2017 at 11:11 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> A survey of s_lock.h shows that we prefer char-width slock_t only on
>>>> these architectures:
>>>>
>>>> x86
>>>> sparc (but not sparcv9, there we use int)
>>>> m68k
>>>> vax

>>> I don't think that's right, because on my MacBook Pro:

>> ... which is an x86, which won't be affected by the proposed change.

> Uh, x86 means 32-bit to me, and this MacBook Pro is definitely x86_64.

Sorry for the confusion. x86 subsumes x86_64 so far as the atomics
code is concerned, and I was following that terminology.

>> Also, since pg_atomic_flag is currently used in a grand total of zero
>> places (other than the test case in regress.c), arguing that making
>> it word-wide will bloat critical data structures is flat wrong.

> Well that just begs the question of whether we should rip it out. If
> it's unused, then, yes, the performance characteristics don't matter,
> but if it's gonna get used for anything important, I maintain that
> both the speed of the implementation and the number of bytes that it
> consumes will be relevant.

[ shrug... ] I have not seen you putting any effort into optimizing
slock_t on non-x86 architectures, where it might make a difference today.
Why all the concern about shaving hypothetical future bytes for
pg_atomic_flag?

But to reduce this to the simplest possible terms: it really does not
matter whether the implementation saves bytes or cycles or anything else,
if it fails to *work*. The existing coding for PPC fails on FreeBSD,
and likely on some other platforms using the same compiler. I'd be the
first to say that that doesn't reflect well on the state of their PPC
port, but it's reality that we ought to be cognizant of. And we've
worked around similar bugs nearby, eg see this bit in atomics.h:

* Given a gcc-compatible xlc compiler, prefer the xlc implementation. The
* ppc64le "IBM XL C/C++ for Linux, V13.1.2" implements both interfaces, but
* __sync_lock_test_and_set() of one-byte types elicits SIGSEGV.

From here it seems like you're arguing that we mustn't give FreeBSD
the identical pass that we already gave IBM, for what is nearly the
same bug.

regards, tom lane