[compiler-rt][builtins] Add tests for atomic builtins support functions

[luismarques]

Summary

Adds some simple sanity checks that the support functions for the atomic
builtins do the right thing. This doesn't test concurrency and memory model
issues.

[asb]

I think there's value in adding these tests (I note libatomic in GCC has a similar set of simple functionality tests), and this seems a sensible way to do it.

I added one inline comment regarding handling the existing builtin. Could you please confirm if the pre-commit test failures are real or spurious build infra related issues?

It might be useful to add some comments alongside the blocks for the atomic fetch_op tests to make it a bit clearer what is being compared.

It would be good if someone who works more actively with atomics could review or at least comment. @jfb @jyknight - any thoughts?

compiler-rt/test/builtins/Unit/atomic_test.c

27

Did you consider using the same #pragma redefine_extname hack as in lib/builtins/atomic.c?

[luismarques]

compiler-rt/test/builtins/Unit/atomic_test.c

27

I discarded that option because the documentation for the #pragma redefine_extname indicates that the asm labels are a better tool all around. See this document for details: https://gcc.gnu.org/onlinedocs/gcc/Symbol-Renaming-Pragmas.html

[luismarques]

Add comment about not resetting the test variables between the tests of different operations in the section that tests __atomic_fetch_<op>_* stuff.

[luismarques]

Correctly gate all instances of 128-bit variables and tests.

[luismarques]

In D81348#2081781, @asb wrote:

Could you please confirm if the pre-commit test failures are real or spurious build infra related issues?

I'm only seeing seemingly unrelated errors. These have persisted even after updating the patch, so it seems it's not something very transient. I think that a buildbot maintainer will have to manually fix some build configuration parameters for these to go away, as these will probably affect other people.

[jfb]

You should probably also test overflow behavior for the RMW variants. Specifically, this clause:

Remarks: For signed integer types, the result is as if the object value and parameters were converted to their corresponding unsigned types, the computation performed on those types, and the result converted back to the signed type. [ Note: There are no undefined results arising from the computation. — end note ]

compiler-rt/test/builtins/Unit/atomic_test.c

126

Do you mean to test alignment properties of the functions? Here you're getting any alignment, and it might be worth checking what the behavior is for aligned as well as unaligned variants of the functions?

154

Same comment on alignment.

197

Here and other places, do you want to test return values?

281

The only requirements here are: The failure argument shall not be memory_order_release nor memory_order_acq_rel.

See: http://wg21.link/p0418

[jyknight]

This isn't sufficient to verify that atomic.c actually works properly (which, BTW, it does not). But at least verifying that the basic single-threaded functionality semantics hold is a good start.

compiler-rt/test/builtins/Unit/atomic_test.c

120

Reading the code below, I was wondering where the "v" variable came from. It looked like it should be a local variable name like "m" so I was confused to find it up here in a #define.

141

I would suggest splitting this into 5 different test functions (for load, store, exchange, compare_exchange, fetch_op) for clarity.

153

All of the size-generic functions (__atomic_load, __atomic_store, __atomic_exchange, __atomic_compare_exchange) are required to handle arbitrary alignments.

It would be good to test that. Maybe add an __attribute__((aligned(16))) to the "data" array to make sure it's aligned. And then, do this loop twice -- once to test aligned, with "data" as the address, and once to test misaligned, with data+1 as the address. (However, atomic.c doesn't currently handle the misaligned case properly, so it may fail, depending on the CPU architecture.)

156

I was going to suggest using gtest and EXPECT_EQ(x, y) instead of if(x != y) abort(). However, given that this code is following the existing style of other tests in this directory (none of which use gtest), I won't push it.

[luismarques]

In D81348#2082596, @jfb wrote:

You should probably also test overflow behavior for the RMW variants. Specifically, this clause:

Remarks: For signed integer types, the result is as if the object value and parameters were converted to their corresponding unsigned types, the computation performed on those types, and the result converted back to the signed type. [ Note: There are no undefined results arising from the computation. — end note ]

Thanks, that's useful to know. I'll update the patch with that, the __atomic_compare_exchange_* tests (which I forgot), the void return types for the atomic store functions, and a conservative alignment for data.
If I misinterpreted your comment about the alignment please clarify (you said "unaligned variants of the *functions*").

compiler-rt/test/builtins/Unit/atomic_test.c

126

Good point about the data being "unaligned". Is it even valid to try to call those atomic functions on unaligned values? I'd normally assume that you need things to be naturally aligned, although the documentation of __atomic_is_lock_free and __atomic_always_lock_free do make me question that a bit. Assuming you need to align things properly, I'll force data to be aligned to N.

197

Oh, that should return void, that was a copy-paste error. Well spotted!

281

Great! My searches missed that report, which nicely clarifies the vague wording on the documentation.

[luismarques]

compiler-rt/test/builtins/Unit/atomic_test.c

153

Thanks, that does clarify jfb's comments. The suggested approach makes sense to me. BTW, where are the requirements about handling unaligned data documented? Not that I'm doubting you, but I had been mostly looking at https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html, which seems omissive about that, so it would be good to know where to look instead.

156

I have no problem with updating the patch to use the gtest macros, but I would like some guidance on whether that is wise to do, given that the other compiler_rt tests don't use it (e.g. are we introducing problematic dependencies, etc.).

[luismarques]

Address review concerns. Various minor fixes and improvements.

[luismarques]

Minor fixes.

[jfb]

compiler-rt/test/builtins/Unit/atomic_test.c

30

Mark src as const.

38

Mark src as const.

133

This should be const.

223

Here you want to do the same alignment thing as above.

231

Ditto on alignment.

[jyknight]

compiler-rt/test/builtins/Unit/atomic_test.c

153

https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary
and also https://llvm.org/docs/Atomics.html#libcalls-atomic

156

I don't actually know if there's an active reason the other code in this directory doesn't use it, or if it's just history. So, I'm afraid I don't know if switching this test to using gtest would cause a problem. Sorry. So maybe best to just keep it like you have. =)

It is used for the sanitizer tests in compiler-rt, *shrug*.

[luismarques]

Add more consts and alignments.

[luismarques]

Some final tweaks to the unaligned tests. I changed my mind about this when I was about to mark some review comments as done. Sorry about the churn!

[luismarques]

compiler-rt/test/builtins/Unit/atomic_test.c

133

Thanks for the suggestion of adding const here. That has already revealed an issue with the GNU libatomic implementation! The following crashes:

struct S {
    int x;
    int y;
    int z;
};

int main() {
    __attribute__((aligned(16))) static const _Atomic struct S a;
    struct S b;
    b = a;
}

The alignment attribute is not needed when compiling with Clang, which already uses that alignment by default, so it's easier to run into this issue when compiling with Clang than with GCC.

GNU bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95722

[jfb]

compiler-rt/test/builtins/Unit/atomic_test.c

133

We've also had issues in LLVM w.r.t. emitting cmpxchg when it's the only way to get a single-copy atomic load for a wide type. I can't find the bugs I have in mind, maybe @t.p.northover remembers? We don't always use FP operations, or non-tearing paired loads, when we see such atomic loads. Sometimes we can't use FP, and sometimes we don't know if paired loads tear or not.

On the GCC side, I know @jwakely was looking at this type of issue earlier this week.

Generally C and C++ aren't very good at this specific issue (whether large const atomic can use cmpxchg, and therefore fault when you load from them).

So I guess for load tests, don't make data const. Only do so for tests where we're not accessing data atomically...

[jyknight]

compiler-rt/test/builtins/Unit/atomic_test.c

133

GCC switched away from directly emitting 16-byte atomics on x86 even with -mcx16, exactly because there's no atomic load instruction. (Clang should really do this too.)

Not only does that break if used on read-only memory, emitting a write when the user expected only a read access ruins the performance of many lock-free algorithms.

Unfortunately, although they stopped emitting the instructions inline, for compatibility with code produced by older GCC versions (and clang), libatomic _must_ continue implementing 16byte atomic loads with cmpxchg16b, rather than using an external mutex.

So....yeah. Sigh.

[jwakely]

compiler-rt/test/builtins/Unit/atomic_test.c

133

What I've been looking at is https://gcc.gnu.org/PR95378 and the related question of why Clang allows the 3rd argument to __atomic_exchange to be volatile T* iff the first argument is volatile T*. It doesn't seem useful (and possibly not even correct) to use a pointer to volatile for that argument.

I don't have anything useful to add here though, sorry.

[lenary]

compiler-rt/test/builtins/Unit/atomic_test.c

188

I think you need a +1 in the declared array length here.

[efriedma]

llvm-commits is not CC'ed on this patch; please resubmit.

[luismarques]

compiler-rt/test/builtins/Unit/atomic_test.c

188

In the unaligned test the loop condition is n < LEN(data) (without the equals, unlike the aligned test), so the largest access we do is LEN(data) - 1. For instance, if data has size 16, we only copy 15 bytes, so we have margin to start a byte further in. Am I missing something?

[luismarques]

This is superseded by D86278, due to issues with subscribing llvm-commits.

[lenary]

compiler-rt/test/builtins/Unit/atomic_test.c

188

No, I missed the comparison was < not <=. Sorry