From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: valgrind versus pg_atomic_init() |
Date: | 2020-06-08 22:02:40 |
Message-ID: | 20200608220240.xyjrvbyfbow72iev@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2020-06-07 00:23:35 -0400, Tom Lane wrote:
> I experimented with running "make check" on ARM64 under a reasonably
> bleeding-edge valgrind (3.16.0). One thing I ran into is that
> regress.c's test_atomic_ops fails; valgrind shows the stack trace
>
> fun:__aarch64_cas8_acq_rel
> fun:pg_atomic_compare_exchange_u64_impl
> fun:pg_atomic_exchange_u64_impl
> fun:pg_atomic_write_u64_impl
> fun:pg_atomic_init_u64_impl
> fun:pg_atomic_init_u64
> fun:test_atomic_uint64
> fun:test_atomic_ops
> fun:ExecInterpExpr
>
> Now, this is basically the same thing as is already memorialized in
> src/tools/valgrind.supp:
>
> # Atomic writes to 64bit atomic vars uses compare/exchange to
> # guarantee atomic writes of 64bit variables. pg_atomic_write is used
> # during initialization of the atomic variable; that leads to an
> # initial read of the old, undefined, memory value. But that's just to
> # make sure the swap works correctly.
> {
> uninitialized_atomic_init_u64
> Memcheck:Cond
> fun:pg_atomic_exchange_u64_impl
> fun:pg_atomic_write_u64_impl
> fun:pg_atomic_init_u64_impl
> }
>
> so my first thought was that we just needed an architecture-specific
> variant of that. But on thinking more about this, it seems like
> generic.h's version of pg_atomic_init_u64_impl is just fundamentally
> misguided. Why isn't it simply assigning the value with an ordinary
> unlocked write? By definition, we had better not be using this function
> in any circumstance where there might be conflicting accesses, so I don't
> see why we should need to tolerate a valgrind exception here. Moreover,
> if a simple assignment *isn't* good enough, then surely the spinlock
> version in atomics.c is 100% broken.
Yea, it could just do that. It seemed slightly easier/clearer, back when
I wrote it, to just use pg_atomic_write* for the initialization, but
this seems enough of a reason to stop doing so. Will change it in all
branches, unless somebody sees a reason to not do so?
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-06-08 22:21:06 | Re: valgrind versus pg_atomic_init() |
Previous Message | Andres Freund | 2020-06-08 21:32:15 | Re: Bump default wal_level to logical |