C11 atomic lock alignment in data_t

[abouteiller]

c11 atomic_flag can be shorter (bool) than the volatile int type used in atomic_external.h, leading to misaligned static structures (like data_t) so, when C11 atomics are enabled, we force the size of atomic_lock_t to be at least one int long, regardless, to match the expected alignment.

[bosilca]

This is ugly. What if you remove the volatile completely ? Or move it outside the union as in

typedef volatile union parsec_atomic_lock_u {
    atomic_flag flag;
    int atomic_external_aligner;
} parsec_atomic_lock_t;

[abouteiller]

the volatile is not useful I removed it and it passes all tests

[bosilca]

I'm really puzzled by this. That a compiler can decide to implement the atomic_flag_t as bool or int, the alignment of the encompassing struct should not change. Moveover, as long as the update/check operations are always done with the same compiler, everything should just work. We always update such variables via accessors defined in parsec library, so this should never be an issue. So, what exactly is the problem here ?

[abouteiller]

the next field in data_t is a uint8_t so it packs tightly with the bool, then the whole rest of the data_t structure is not aligned in memory in the same way when looked at from a file that includes atomic-external.h versus atomic-c11.h. That ends up causing crashes when we dereference fields in the data_t, and obtain garbage from fields with a differing offsetof. That can be seen for example when we declare data_t in the lapack reshaping functions in dplasma (they are in atomic-external packing shape), and then read the values of the data_t in parsec (then in atomic-c11 packed shape). This has been observed and confirmed in gdb, and the patch fixes this issue.

[bosilca]

According to the code in data_internal.h the next field in the parsec_data_t is parsec_data_key_t aka uint64_t, aligned to 8.

[therault]

Interesting / facetious :) We saw that behavior while debugging the PR #671 where indeed the parsec_data_key_t field is removed from the definition of parsec_data_t, making this bug manifest (https://github.com/ICLDisco/parsec/pull/671/files#diff-dbde8517930fb2ca1ac0555fc9e18cc92a7a282a97a2e20fa222c7fc1d73fd16) because then the next field is a int8_t.

IMHO, it is fragile to rely on the alignment of the next field in some structure to guarantee the types with atomic-external.h and without are compatible. I prefer the (admittedly not very aesthetic) solution of forcing the base type to be compatible with what we expose in atomic-external.h