[MDEV-18677] clang 7 fails to compile innodb - Jira

Details

Type: Bug
Status: Closed (View Workflow)
Priority: Major
Resolution: Fixed
Affects Version/s: 10.4(EOL)
Fix Version/s: 10.4.3
Component/s: Compiling, Storage Engine - InnoDB
Labels:
None

Description

C:\PROGRA~1\LLVM\bin\clang-cl.exe /nologo -TP -DBTR_CUR_ADAPT -DBTR_CUR_HASH_ADAPT -DCOMPILER_HINTS -DHAVE_CONFIG_H -DMUTEX_EVENT -DNOMINMAX -DWIN32_LEAN_AND_MEAN -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_WIN32_WINNT=0x0A00 -D_WINDOWS -D_WIN_ -Iinclude -I..\storage\innobase\include -I..\storage\innobase\handler -I..\libbinlogevents\include -I..\include -I..\sql -Ipcre -I..\pcre -I..\extra\yassl\include -I..\extra\yassl\taocrypt\include -I..\zlib -Izlib /DWIN32 /D_WINDOWS /W3 /GR /EHsc -Wno-unused-parameter -Wno-unused-command-line-argument -Wno-pointer-sign -Wno-deprecated-register -Wno-missing-braces -Wno-unused-function -msse4.2 /we4099 /we4700 /we4311 /we4477 /we4302 /we4090 /MD /Z7 /O2 /Ob1 /DNDEBUG -DDBUG_OFF /wd4065 /showIncludes /Fostorage\innobase\CMakeFiles\innobase.dir\sync\sync0rw.cc.obj /Fdstorage\innobase\CMakeFiles\innobase.dir\innobase.pdb -c ..\storage\innobase\sync\sync0rw.cc
..\storage\innobase\sync\sync0rw.cc(141,18): error: call to implicitly-deleted default constructor of 'rw_lock_stats_t'
rw_lock_stats_t rw_lock_stats;
^
..\storage\innobase\include\sync0rw.h(46,19): note: default constructor of 'rw_lock_stats_t' is implicitly deleted because field 'rw_s_spin_wait_count' has a deleted default constructor
int64_counter_t rw_s_spin_wait_count;
^
..\storage\innobase\include/ut0counter.h(116,51): note: default constructor of 'ib_counter_t<long long, 64>' is implicitly deleted because field 'm_counter' has a deleted default constructor
MY_ALIGNED(CACHE_LINE_SIZE) ib_counter_element_t m_counter[N];
^
..\storage\innobase\include/ut0counter.h(110,21): note: default constructor of 'ib_counter_element_t' is implicitly deleted because variant field 'value' has a non-trivial default constructor
std::atomic<Type> value;
^
1 error generated.

Attachments

Issue Links

relates to

MDEV-17441 InnoDB transition to C++11 atomics

Closed

Activity

Ascending order - Click to sort in descending order

Marko Mäkelä added a comment - 2019-02-21 13:24

For what it is worth, I have been successfully using clang 7.0.1 and 8.0.0 on Debian GNU/Linux.
Could it be that the definition of std::atomic differs between the two platforms? On my system, /usr/include/c++/8/atomic (from libstdc++-8-dev, possibly something to do with GCC 8.2.0) defines default constructors and destructors:

namespace std _GLIBCXX_VISIBILITY(default)

_GLIBCXX_BEGIN_NAMESPACE_VERSION

…

  template<typename _Tp>

    struct atomic;

  /// atomic<bool>

  // NB: No operators or fetch-operations for this type.

  template<>

  struct atomic<bool>

  private:

    __atomic_base<bool> _M_base;

  public:

    atomic() noexcept = default;

    ~atomic() noexcept = default;

    atomic(const atomic&) = delete;

    atomic& operator=(const atomic&) = delete;

    atomic& operator=(const atomic&) volatile = delete;

    constexpr atomic(bool __i) noexcept : _M_base(__i) { }

…

};

…

  template<typename _Tp>

    struct atomic

…

      atomic() noexcept = default;

      ~atomic() noexcept = default;

      atomic(const atomic&) = delete;

      atomic& operator=(const atomic&) = delete;

      atomic& operator=(const atomic&) volatile = delete;

      constexpr atomic(_Tp __i) noexcept : _M_i(__i) { }

wlad, how does the definition look like in your environment, and what does the standard say about the existence of default constructors and destructors?

Marko Mäkelä added a comment - 2019-02-21 13:24 For what it is worth, I have been successfully using clang 7.0.1 and 8.0.0 on Debian GNU/Linux. Could it be that the definition of std::atomic differs between the two platforms? On my system, /usr/include/c++/8/atomic (from libstdc++-8-dev , possibly something to do with GCC 8.2.0) defines default constructors and destructors: namespace std _GLIBCXX_VISIBILITY( default ) { _GLIBCXX_BEGIN_NAMESPACE_VERSION … template < typename _Tp> struct atomic; /// atomic<bool> // NB: No operators or fetch-operations for this type. template <> struct atomic< bool > { private : __atomic_base< bool > _M_base; public : atomic() noexcept = default ; ~atomic() noexcept = default ; atomic( const atomic&) = delete ; atomic& operator=( const atomic&) = delete ; atomic& operator=( const atomic&) volatile = delete ; constexpr atomic( bool __i) noexcept : _M_base(__i) { } … }; … template < typename _Tp> struct atomic { … atomic() noexcept = default ; ~atomic() noexcept = default ; atomic( const atomic&) = delete ; atomic& operator=( const atomic&) = delete ; atomic& operator=( const atomic&) volatile = delete ; constexpr atomic(_Tp __i) noexcept : _M_i(__i) { } wlad , how does the definition look like in your environment, and what does the standard say about the existence of default constructors and destructors?

Vladislav Vaintroub added a comment - 2019-02-21 13:44

Why would we use a default constructor in a counter? Do we want counter to be initialized to any value?

Vladislav Vaintroub added a comment - 2019-02-21 13:44 Why would we use a default constructor in a counter? Do we want counter to be initialized to any value?

Vladislav Vaintroub added a comment - 2019-02-21 14:04

#ifndef _ATOMIC_HAS_NO_SPECIALIZATION

// TEMPLATE SPECIALIZATION atomic<_ITYPE>

template <>

struct atomic<_ITYPE> : _ATOMIC_ITYPE { // template specialization that manages values of _ITYPE atomically

    using value_type      = _ITYPE;

    using difference_type = _ITYPE;

#ifdef __clang__ // TRANSITION, VSO#406237

    constexpr atomic() noexcept : _ATOMIC_ITYPE() {}

#else // ^^^ no workaround ^^^ // vvv workaround vvv

    atomic() = default;

#endif // VSO#406237

    constexpr atomic(_ITYPE _Val) noexcept

        : _ATOMIC_ITYPE{(_ATOMIC_UINT) _Val}

Vladislav Vaintroub added a comment - 2019-02-21 14:04 #ifndef _ATOMIC_HAS_NO_SPECIALIZATION // TEMPLATE SPECIALIZATION atomic<_ITYPE> template <> struct atomic<_ITYPE> : _ATOMIC_ITYPE { // template specialization that manages values of _ITYPE atomically using value_type = _ITYPE; using difference_type = _ITYPE; #ifdef __clang__ // TRANSITION, VSO#406237 constexpr atomic() noexcept : _ATOMIC_ITYPE() {} #else // ^^^ no workaround ^^^ // vvv workaround vvv atomic() = default; #endif // VSO#406237 constexpr atomic(_ITYPE _Val) noexcept : _ATOMIC_ITYPE{(_ATOMIC_UINT) _Val}

Vladislav Vaintroub added a comment - 2019-02-21 14:19

marko, that https://github.com/MariaDB/server/commit/f80bfae822b588614bf880887b3bb61adc4185b8 would be my fix. I do not even have to initialize atomic value, but I believe it should be 0 in the given usage. The usage of union was also questionable, why to use padding if there is MY_ALIGN.

Vladislav Vaintroub added a comment - 2019-02-21 14:19 marko , that https://github.com/MariaDB/server/commit/f80bfae822b588614bf880887b3bb61adc4185b8 would be my fix. I do not even have to initialize atomic value, but I believe it should be 0 in the given usage. The usage of union was also questionable, why to use padding if there is MY_ALIGN.

Marko Mäkelä added a comment - 2019-02-21 14:22

The problem seems to be this one:

..\storage\innobase\include/ut0counter.h(110,21): note: default constructor of 'ib_counter_element_t' is implicitly deleted because variant field 'value' has a non-trivial default constructor

and the relevant code:

template <typename Type, int N = IB_N_SLOTS>

struct ib_counter_t {

// …

	/** Atomic which occupies whole CPU cache line */

	union ib_counter_element_t {

		std::atomic<Type> value;

		byte padding[CACHE_LINE_SIZE];

};

	static_assert(sizeof(ib_counter_element_t) == CACHE_LINE_SIZE, "");

	/** Array of counter elements */

	MY_ALIGNED(CACHE_LINE_SIZE) ib_counter_element_t m_counter[N];

};

It seems that we would have to define a default constructor for the union type, which we need because we want each element of m_counter to sit in its own cache line. Would the following work? (I only tested that it does not break the build with clang 8; will test with GCC as well.)

 diff --git a/storage/innobase/include/ut0counter.h b/storage/innobase/include/ut0counter.h

index 1fc2c297e8d..612f9d43c95 100644

--- a/storage/innobase/include/ut0counter.h

+++ b/storage/innobase/include/ut0counter.h

@@ -109,6 +109,7 @@ struct ib_counter_t {

 	union ib_counter_element_t {

 		std::atomic<Type> value;

 		byte padding[CACHE_LINE_SIZE];

+		ib_counter_element_t() : value() {}

};

 	static_assert(sizeof(ib_counter_element_t) == CACHE_LINE_SIZE, "");

Marko Mäkelä added a comment - 2019-02-21 14:22 The problem seems to be this one: ..\storage\innobase\include/ut0counter.h(110,21): note: default constructor of 'ib_counter_element_t' is implicitly deleted because variant field 'value' has a non-trivial default constructor and the relevant code: template < typename Type, int N = IB_N_SLOTS> struct ib_counter_t { // … /** Atomic which occupies whole CPU cache line */ union ib_counter_element_t { std::atomic<Type> value; byte padding[CACHE_LINE_SIZE]; }; static_assert( sizeof (ib_counter_element_t) == CACHE_LINE_SIZE, "" ); /** Array of counter elements */ MY_ALIGNED(CACHE_LINE_SIZE) ib_counter_element_t m_counter[N]; }; It seems that we would have to define a default constructor for the union type, which we need because we want each element of m_counter to sit in its own cache line. Would the following work? (I only tested that it does not break the build with clang 8; will test with GCC as well.) diff --git a/storage/innobase/include/ut0counter.h b/storage/innobase/include/ut0counter.h index 1fc2c297e8d..612f9d43c95 100644 --- a/storage/innobase/include/ut0counter.h +++ b/storage/innobase/include/ut0counter.h @@ -109,6 +109,7 @@ struct ib_counter_t { union ib_counter_element_t { std::atomic<Type> value; byte padding[CACHE_LINE_SIZE]; + ib_counter_element_t() : value() {} }; static_assert(sizeof(ib_counter_element_t) == CACHE_LINE_SIZE, "");

Marko Mäkelä added a comment - 2019-02-21 15:19

wlad, thanks for your patch. I pushed a variation of that Using struct instead of union is much cleaner. And I am not at all sure if a union is allowed to have a constructor.

I only removed the use of the default constructor, because all current usage of this template is for srv_stats and rw_lock_stats, which are allocated statically and should be zero-initialized. I believe that only fairly recent compilers can optimize away zero-initialization of memory that is allocated from the BSS. Because the two global variables in question are large, having the constructor could cause a lot of unnecessary initialization code to be emitted on some platforms.

I added a comment that says that the memory is not explicitly initialized and explains that the instantiated objects are guaranteed to be zero-initialized.

Marko Mäkelä added a comment - 2019-02-21 15:19 wlad , thanks for your patch. I pushed a variation of that Using struct instead of union is much cleaner. And I am not at all sure if a union is allowed to have a constructor. I only removed the use of the default constructor, because all current usage of this template is for srv_stats and rw_lock_stats , which are allocated statically and should be zero-initialized. I believe that only fairly recent compilers can optimize away zero-initialization of memory that is allocated from the BSS. Because the two global variables in question are large, having the constructor could cause a lot of unnecessary initialization code to be emitted on some platforms. I added a comment that says that the memory is not explicitly initialized and explains that the instantiated objects are guaranteed to be zero-initialized.

People

Assignee:: Marko Mäkelä

Reporter:: Vladislav Vaintroub

Votes:: 0 Vote for this issue

Watchers:: 3 Start watching this issue

Dates

Created:: 2019-02-21 10:41

Updated:: 2019-02-21 15:21

Resolved:: 2019-02-21 15:19

Git Integration

Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.

MariaDB Server