Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSGPACK_ENABLE_GCC_CXX_ATOMIC - Failed #905

Closed
wfjm opened this issue Jul 31, 2020 · 3 comments
Closed

MSGPACK_ENABLE_GCC_CXX_ATOMIC - Failed #905

wfjm opened this issue Jul 31, 2020 · 3 comments

Comments

@wfjm
Copy link

wfjm commented Jul 31, 2020

When running cmake on the cpp_master branch on Ubuntu 18.04 I get a

    -- Performing Test MSGPACK_ENABLE_GCC_CXX_ATOMIC
    -- Performing Test MSGPACK_ENABLE_GCC_CXX_ATOMIC - Failed

The test code in CMakeLists.txt reads

#include <bits/atomicity.h>
int atomic_sub(int i) { return __gnu_cxx::__exchange_and_add(&i, -1) - 1; }
int atomic_add(int i) { return __gnu_cxx::__exchange_and_add(&i, 1) + 1; }
int main(int argc, char * argv[])
{
    atomic_sub(1);
    atomic_add(1);
}

The culprit is that since quite some time the header is under

#include <ext/atomicity.h>

Looking through some old systems is see that

  • up to gcc 4.1.1 I find bits/atomicity.h
  • after gcc 4.6.3 I find ext/atomicity.h

so the transition was somewhere between gcc 4.1.1 and 4.6.3. Ubuntu 18.04 has gcc version 7.5.0

The code compiles and tests without atomicity.h, hard to judge whether there are performance impacts.
In case atomic actions are important, it seems to me that std::atomic is the natural choice these days.

In closing the steps to reproduce (on an Ubuntu 18.04 system, likely on all current Ubuntu/Debian systems):

  git clone https://github.com/msgpack/msgpack-c.git
  cd msgpack-c
  git checkout cpp_master 
  git checkout 70912ff

  mkdir build
  cd build
  cmake -DMSGPACK_CXX17=ON ..
@redboltz
Copy link
Contributor

In case atomic actions are important, it seems to me that std::atomic is the natural choice these days.

Yes. since C++11, msgpack-c uses std::atomic.
See

inline void incr_count(void* buffer)
{
#if defined(MSGPACK_USE_CPP03)
_msgpack_sync_incr_and_fetch(reinterpret_cast<volatile _msgpack_atomic_counter_t*>(buffer));
#else // defined(MSGPACK_USE_CPP03)
++*reinterpret_cast<std::atomic<unsigned int>*>(buffer);
#endif // defined(MSGPACK_USE_CPP03)
}

For C++03, msgpack-c provides _msgpack_sync_incr_and_fetch. It is not a standard way so there are various compiler support.

#ifdef _WIN32
# if defined(_KERNEL_MODE)
# define _msgpack_atomic_counter_header <ntddk.h>
# else
# define _msgpack_atomic_counter_header <windows.h>
# if !defined(WIN32_LEAN_AND_MEAN)
# define WIN32_LEAN_AND_MEAN
# endif /* WIN32_LEAN_AND_MEAN */
# endif
typedef long _msgpack_atomic_counter_t;
#if defined(_AMD64_) || defined(_M_X64) || defined(_M_ARM64)
# define _msgpack_sync_decr_and_fetch(ptr) _InterlockedDecrement(ptr)
# define _msgpack_sync_incr_and_fetch(ptr) _InterlockedIncrement(ptr)
#else
# define _msgpack_sync_decr_and_fetch(ptr) InterlockedDecrement(ptr)
# define _msgpack_sync_incr_and_fetch(ptr) InterlockedIncrement(ptr)
#endif
#elif defined(__GNUC__) && ((__GNUC__*10 + __GNUC_MINOR__) < 41)
# define _msgpack_atomic_counter_header "msgpack/gcc_atomic.hpp"
#else
typedef unsigned int _msgpack_atomic_counter_t;
# define _msgpack_sync_decr_and_fetch(ptr) __sync_sub_and_fetch(ptr, 1)
# define _msgpack_sync_incr_and_fetch(ptr) __sync_add_and_fetch(ptr, 1)
#endif

https://github.com/msgpack/msgpack-c/blob/6598c6228fb9ce798a7bc1e6300cc8af514af0a7/include/msgpack/gcc_atomic.hpp
is only for older compilers.

However the top level CMakeLists.txt always check it. And outputs Failed message.
I think that cmake process itself is successfully finished even if Failed is output.

Is that right ?

If it is, the problem is not so critical but msgpack-c shouldn't output misleading message at the cmake process.
I will investigate it.

redboltz added a commit to redboltz/msgpack-c that referenced this issue Jul 31, 2020
Added compiler version cheking for atomic test for older compilers on cmake.
@redboltz
Copy link
Contributor

redboltz commented Aug 1, 2020

#906 should fix the issue.

redboltz added a commit that referenced this issue Aug 7, 2020
@redboltz
Copy link
Contributor

redboltz commented Aug 7, 2020

#906 merged.

@redboltz redboltz closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants