[MDEV-18677] clang 7 fails to compile innodb Created: 2019-02-21  Updated: 2019-02-21  Resolved: 2019-02-21

Status: Closed
Project: MariaDB Server
Component/s: Compiling, Storage Engine - InnoDB
Affects Version/s: 10.4
Fix Version/s: 10.4.3

Type: Bug Priority: Major
Reporter: Vladislav Vaintroub Assignee: Marko Mäkelä
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Relates
relates to MDEV-17441 InnoDB transition to C++11 atomics Closed

 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.

 Comments   
Comment by Marko Mäkelä [ 2019-02-21 ]

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?

Comment by Vladislav Vaintroub [ 2019-02-21 ]

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

Comment by Vladislav Vaintroub [ 2019-02-21 ] 

#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}

Comment by Vladislav Vaintroub [ 2019-02-21 ]

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.

Comment by Marko Mäkelä [ 2019-02-21 ]

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, "");

Comment by Marko Mäkelä [ 2019-02-21 ]

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.

Generated at Thu Feb 08 08:45:54 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.