Heikki, Peter, thanks a lot for code review!
> What's going on here? Surely pg_atomic_init_u64() should initialize
> the value?
It's because of how pg_atomic_exchange_u64_impl is implemented:
```
while (true)
{
old = ptr->value; /* <-- reading of uninitialized value! */
if (pg_atomic_compare_exchange_u64_impl(ptr, &old, xchg_))
break;
}
```
Currently pg_atomic_init_u64 works like this:
pg_atomic_init_u64
`- pg_atomic_init_u64_impl
`- pg_atomic_write_u64_impl
`- pg_atomic_exchange_u64_impl
I suspect there is actually no need to make an atomic exchange during
initialization of an atomic variable. Regular `mov` should be enough
(IIRC there is no need to do `lock mov` since `mov` is already atomic).
Anyway I don't feel brave enough right now to mess with atomic
operations since it involves all sort of portability issues. So I
removed this change for now.
> Why does MemorySanitizer complain about that? Calling stat(2) is
> supposed to fill in all the fields we look at, right?
> In addition to what Heikki wrote, I think the above is not necessary.
It's been my observation that MemorySanitizer often complains on passing
structures with uninitialized padding bytes to system calls. I'm not
sure whether libc really reads/copies structures in these cases or
MemorySanitizer doesn't like the idea in general.
Although it's true that maybe MemorySanitizer it too pedantic in these
cases, in some respect it's right. Since operating system is a black box
from our perspective (not mentioning that there are _many_ OS'es that
change all the time) in general case passing a structure with
uninitialized padding bytes to a system call can in theory cause a
non-repeatable behavior. For instance if there are caching and hash
calculation involved.
Also, as Chapman noted previously [1], according to PostgreSQL
documentation using structures with uninitialized padding bytes should
be avoided anyway.
I strongly believe that benefits of passing all MemorySanitizer checks
(possibility of discovering many complicated bugs automatically in this
case) many times outweighs drawbacks of tiny memset's overhead in these
concrete cases.
> I think this goes too far. You're zeroing all palloc'd memory, even
> if it's going to be passed to palloc0(), and zeroed there. It might
> even silence legitimate warnings, if there's code somewhere that
> does palloc(), and accesses some of it before initializing. Plus
> it's a performance hit.
Completely agree, my bad. I removed these changes.
> Right after this, we have:
> VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg));
> Do we need both?
Apparently we don't. I removed VALGRIND_MAKE_MEM_DEFINED here since now
there are no uninitialized padding bytes in &msg.
Corrected patch is attached. If you have any other ideas how it could
be improved please let me know!
[1]
https://www.postgresql.org/message-id/56EFF347.20500%40anastigmatix.net
--
Best regards,
Aleksander Alekseev
