Page MenuHomePhabricator

Patch llvm bug 41033 concerning atomicity of statement expressions
AbandonedPublic

Authored by mibintc on Mar 13 2019, 9:51 AM.

Details

Summary

Clang considers the type of a statement expression that returns the value of an _Atomic(ty) to be atomic, and this makes the statement expression incompatible with other expressions of type ty, for example in assignment or comparison. This patch drops the atomic attribute before creating the StmtExpr.

I noticed that unlike gcc, clang doesn't allow StmtExpr on the left hand side of an assignment, I agree with clang.

I checked with Clark Nelson about the legitimacy of dropping the atomicity, he said "Atomicity has to do with the way data is transferred between a processor and its memory. Once it has been loaded, it's just data."

Diff Detail

Repository
rC Clang

Event Timeline

mibintc created this revision.Mar 13 2019, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 9:51 AM
Herald added subscribers: jdoerfert, jfb. · View Herald Transcript
jfb requested changes to this revision.Mar 13 2019, 9:56 AM

I think you also want to test C++ std::atomic as well as regular volatile.

test/Sema/atomic-expr-stmt.c
7

Why is there a store to a here?

9

What's in %tmp? I'd expect the value loaded from a to be stored to b.

This revision now requires changes to proceed.Mar 13 2019, 9:56 AM
jfb added a subscriber: __simt__.Mar 13 2019, 9:56 AM
jfb added a comment.Mar 13 2019, 10:08 AM

Thinking some more, this is really a silly gotcha in code. We should probably follow what we're about to do with wg21.link/P1152 and ban chaining. Statement expressions should therefore not be allowed to return _Atomic, std::atomic, nor volatile.

jfb added a subscriber: jwakely.Mar 13 2019, 10:12 AM

From an offline discussion, I'm told that @jwakely uses this in GCC headers and expects some semantics from it? Jonathan, why?!?!

mibintc marked 2 inline comments as done.Mar 13 2019, 11:16 AM

Thanks for test suggestions, will follow up

test/Sema/atomic-expr-stmt.c
7

Yes the IR is odd, I will rewrite with a as global _Atomic int, not a parameter

9

It does seem like the "store atomic" to tmp shouldn't exist. The %tmp is the value returned by the StmtExpr. Since the atomic load has already occurred, the value returned by StmtExpr could just be %atomic-load. If I compare this IR to a test case without the StmtExpr, there's just the atomic load of a which is then stored into b. I think the value of the atomic-load is still hanging on to the atomic attribute but it should be dropped.

I changed the test like this, and the IR follows _Atomic int a; void test_assign(int b) {

// assignment is OK
b = ({a;});

}

; Function Attrs: noinline nounwind optnone define void @test_assign(i32 %b) #0 {
entry:

%b.addr = alloca i32, align 4
%tmp = alloca i32, align 4
store i32 %b, i32* %b.addr, align 4
%atomic-load = load atomic i32, i32* @a seq_cst, align 4
store atomic i32 %atomic-load, i32* %tmp seq_cst, align 4
%0 = load i32, i32* %tmp, align 4
store i32 %0, i32* %b.addr, align 4
ret void

}

In D59307#1427663, @jfb wrote:

Thinking some more, this is really a silly gotcha in code. We should probably follow what we're about to do with wg21.link/P1152 and ban chaining. Statement expressions should therefore not be allowed to return _Atomic, std::atomic, nor volatile.

If the atomic load which is the value of the StmtExpr drops the atomic attribute then this would trivially be true, and there would be no need to add a restriction. According to what Clark says, once the value is loaded it's just data.

In D59307#1427665, @jfb wrote:

From an offline discussion, I'm told that @jwakely uses this in GCC headers and expects some semantics from it? Jonathan, why?!?!

Yes this is how I stumbled into the issue.

This is my concern here:
https://godbolt.org/z/icS4fa
The patch will change template instantiation.

GCC doesn't seem to allow using _Atomic in C++ mode, which is perhaps a necessary part of this solution? I see we already consider overload sets with int and _Atomic(int) to be ambiguous (in addition to not considering conversion operators of _Atomic(int) at all??), so it is limited to type deduction. I could see this causing problems, since specializations of templates are allowed on int and _Atomic(int) despite not being used in overload sets; https://godbolt.org/z/oMAvpz

In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

jfb added a comment.Mar 13 2019, 1:49 PM
In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

What I had in mind with atomic isn't relevant, because it would try to call atomic(const atomic&) = delete;. Your test case here isn't something I'm worried about.

volatile is still and issue.

I'm still not sure this is something we want.

In D59307#1428145, @jfb wrote:
In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

What I had in mind with atomic isn't relevant, because it would try to call atomic(const atomic&) = delete;. Your test case here isn't something I'm worried about.

volatile is still and issue.

I'm still not sure this is something we want.

Now I think the problem needs to be fixed at the atomic load, not in this place. Thanks for pointing out the odd use of tmp. I'm going to abandon this patch.

This is my concern here:
https://godbolt.org/z/icS4fa
The patch will change template instantiation.

GCC doesn't seem to allow using _Atomic in C++ mode, which is perhaps a necessary part of this solution? I see we already consider overload sets with int and _Atomic(int) to be ambiguous (in addition to not considering conversion operators of _Atomic(int) at all??), so it is limited to type deduction. I could see this causing problems, since specializations of templates are allowed on int and _Atomic(int) despite not being used in overload sets; https://godbolt.org/z/oMAvpz

Recognizing _Atomic in c++ mode is an extension right? Probably it needs more thought, for example the test case you provided. Thank you

mibintc abandoned this revision.Mar 14 2019, 9:08 AM

The bug should be fixed at the atomic-load (drop _Atomic qualifier) then this patch won't be needed in StmtExpr