__sync_bool_compare_and_swap build error

[llvmbot]

Bugzilla Link	11280
Version	trunk
OS	Linux
Reporter	LLVM Bugzilla Contributor
CC	@DougGregor,@efriedma-quic,@rjmccall

Extended Description

I've been keeping up with Clang/LLVM trunk - shortly after 139216 (which I am assuming is the commit that broke it), applications using OpenSceneGraph fail to build due to a compile error in the OpenThreads/Atomic header. I've stripped down that header to just the minimal portion that causes the error.

void* volatile _ptr;

bool assign(void ptrNew, const void const ptrOld) { return __sync_bool_compare_and_swap(&_ptr, ptrOld, ptrNew); }

If you compile this with clang++ Atomic.cpp -o Atomic.o, you get the following error:

Atomic.cpp:19:48: error: cannot initialize a parameter of type 'void ' with an lvalue of type 'const void const' return __sync_bool_compare_and_swap(&_ptr, ptrOld, ptrNew); ^~

This did work sometime before 139216, and works with a wide range of GCC versions - that code hasn't been touched since 2008 in the upstream project.

Wondering if this is a Clang error or an error in OpenThreads - I would suspect since it works with GCC and it's a GCC intrinsic, it's a Clang error.

[llvmbot]

If it is meaningful at all, this minimal example still fails with the same error on:

clang version 8.0.0-svn343946-1~exp1+0~20181007200936.2062~1.gbpea43b1 (trunk) Target: x86_64-pc-linux-gnu

Don't know if this is bad upstream code or not, or if the upstream where I found this still uses it, I was just trying to find another bug I commented on and discovered this one I reported years ago, and thus figured I'd try the test case again.

[efriedma-quic]

Martin, it looks like you're seeing a different issue; please file a new bug and attach preprocessed source for the file with the error.

[llvmbot]

Just so you know. I found a work-around for my problem. Wrapping the calls into an inline function seems to help the compiler to select the correct function.

[llvmbot]

I got a similar problem, but with size_t (unsigned long) data-types on the 3.0 release (C++0x enabled). It occurs both with sync_bool_compare_and_swap as well as with sync_fetch_and_add. All parameters are of the same type. The code compiles and executes correctly on gcc. Fun fact: I cannot reproduce the problem in a small program. There it seems to work.

Any idea how I can work around this? This is a real show-stopper for me right now...

error: cannot initialize a parameter of type 'volatile long long ' with an lvalue of type 'size_t ' (aka 'unsigned long *') if(SIZET_CAS(upper_bound, old_ub, cut)) {

./pheet/misc/atomics_clang.h:20:66: note: expanded from:
#define SIZET_CAS(p, old_v, new_v)      (__sync_bool_compare_and_swap(p, old_v, new_v))

error: cannot initialize a parameter of type 'volatile long long *' with an rvalue of type 'size_t *' (aka 'unsigned long *')
                SIZET_ATOMIC_ADD(&(stack_element->num_finished_remote), 1);
                ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./pheet/misc/atomics_clang.h:27:56: note: expanded from:
#define SIZET_ATOMIC_ADD(p, val)        (__sync_fetch_and_add(p, val))

[llvmbot]

So is this something that needs to be fixed in the upstream project (if so, how? This is the code in its fuller context: http://www.openscenegraph.org/projects/osg/browser/OpenSceneGraph/trunk/include/OpenThreads/Atomic ), or will this be resolved in Clang?

This page doesn't seem to specify a particular behavior precisely: http://gcc.gnu.org/onlinedocs/gcc/Atomic-Builtins.html The Itanium guide seems to be more explicit that all those types are the same, but doesn't mention void* as an allowable type. (Would casting to intptr_t keep this atomic?)

Thanks!

[rjmccall]

Er, right, of course it does. Not sure what I was thinking there.

[efriedma-quic]

Nor would I want us to accept that with the semantics of the float being turned to an int.

We actually do accept that at the moment; the following compiles without any warning (well, at least it doesn't unless you turn -Wconversion on):

int x; void a(double d, double e) { __sync_bool_compare_and_swap(&x, d, e); }

[rjmccall]

Interesting. Passing an over-qualified argument here is actually safe because the argument is only used as a comparison operand. I very much doubt that GCC is doing that sophisticated of an analysis, though; Clang certainly wasn't. And it's pretty much only qualification conversions that are legal here; (*intptr == 1.0f) type-checks but isn't possible to do atomically. Nor would I want us to accept that with the semantics of the float being turned to an int.

I think making this work well would be a lot of complexity for a very small amount of value.

[efriedma-quic]

This is caused by r141170; I think it's an unintended consequence, though.

[wheatman]

This still gives an error on post 16 trunk(f05b58a9468cc2990678e06bc51df56b30344807) https://godbolt.org/z/35G9aEb17

code

void* _ptr;

bool assign(void* ptrNew, const void* ptrOld) {
    return __sync_bool_compare_and_swap(&_ptr, ptrOld, ptrNew);
}

error

<source>:4:48: error: cannot initialize a parameter of type 'void *' with an lvalue of type 'const void *'
    4 |     return __sync_bool_compare_and_swap(&_ptr, ptrOld, ptrNew);
      |                                                ^~~~~~
1 error generated.
Compiler returned: 1

[llvmbot]

@llvm/issue-subscribers-clang-frontend