Remove the restriction of ten types on AligedCharArrayUnion

[spyffe]

Summary

This patch uses C++11 parameter packs and constexpr functions to allow AlignedCharArrayUnion to hold an arbitrary number of types.

[davide]

Can you add an unittest to make sure we handle correctly > 10 arguments?

[spyffe]

Updated to add static and runtime unit tests.

[dblaikie]

Hmm - did have some comments, but I think, maybe, all the supported compilers support std::aligned_union now, so switching to that might be an/the right option now? *hopeful*

include/llvm/Support/AlignOf.h

131 ↗

(On Diff #83515)

Leave a non-doxy comment about how std::aligned_union could be used once LLVM's supported compilers support it. (GCC 4.8 is what we'd need, I think - oh, 4.8 is our minimum)

unittests/ADT/AlignOfTest.cpp

31–34 ↗

(On Diff #83515)

These shouldn't need to be in a class like this, should they?

36–41 ↗

(On Diff #83515)

Not sure this test is needed/covers anything - tests that memset works, really? But, yeah... not sure what the right/appropriate testing would be for this.

38 ↗

(On Diff #83515)

Should that be sizeof(MyU.buffer) rather than sizeof(U)? (it could have extra padding, etc - I guess not with this specific set of types)

[davide]

I agree with David here. Can you check if std::aligned_union actually works with 4.8 and the minimum version of MSVC we support (I think 2015) and switch to it instead?

[spyffe]

According to the GCC 5 changelist, support for std::aligned_union was introduced with GCC 5.
I was able to verify with Compiler Explorer that std::aligned_union is unsupported (as in, not found) by 4.9.5. It is supported by 5.1.

According to Microsoft, Visual Studio 2015 supports std::aligned_union.
I was able to verify with "Compile Visual Studio C++ Online" that Visual C++ 2015 supports it.

[davide]

In D28429#640011, @spyffe wrote:

According to the GCC 5 changelist, support for std::aligned_union was introduced with GCC 5.
I was able to verify with Compiler Explorer that std::aligned_union is unsupported (as in, not found) by 4.9.5. It is supported by 5.1.

Oh, too bad.

According to Microsoft, Visual Studio 2015 supports std::aligned_union.
I was able to verify with "Compile Visual Studio C++ Online" that Visual C++ 2015 supports it.

Thanks for checking.

I'm fine with this going in as long as David is happy.

[davide]

Where do you actually plan to use this? In LLVM proper or in some downstream project?

[dblaikie]

Please include a comment in the header/near/in the definition of AlignedCharArrayUnion about the minimum compiler version before we can switch to std::aligned_union.

[spyffe]

In D28429#640018, @davide wrote:

Where do you actually plan to use this? In LLVM proper or in some downstream project?

My plan was to use this to implement std::variant (a C++17 feature) on the cheap inside LLVM. I mentioned wanting to implement a tagged union and Lang Hames said it would be useful in several places. Hence the review.

[spyffe]

Added a comment at David's request, and moved the test into the Support/AlignOf unit tests. Also removed the memcpy test.

[spyffe]

This failed to build under Visual Studio:

C:\PROGRA~2\MICROS~1.0\VC\bin\AMD64_~2\cl.exe   /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Support -IC:\b\slave\sanitizer-windows\llvm\lib\Support -Iinclude -IC:\b\slave\sanitizer-windows\llvm\include /DWIN32 /D_WINDOWS   /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4324 -w14062 -we4238 /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /MD /O2 /Ob2   -UNDEBUG  /EHs-c- /GR- /showIncludes /Folib\Support\CMakeFiles\LLVMSupport.dir\CommandLine.cpp.obj /Fdlib\Support\CMakeFiles\LLVMSupport.dir\ /FS -c C:\b\slave\sanitizer-windows\llvm\lib\Support\CommandLine.cpp
C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(135): error C2027: use of undefined type 'llvm::detail::AlignerImpl<Ts...>'
C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(135): note: see declaration of 'llvm::detail::AlignerImpl<Ts...>'
C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(136): note: see reference to class template instantiation 'llvm::AlignedCharArrayUnion<Ts...>' being compiled
ninja: build stopped: subcommand failed.

I did some restricted testing with http://rextester.com/l/cpp_online_compiler_visual but it looks like that wasn't sufficient.

[chapuni]

Is it compatible to -m32? See also; http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/33628

[mgorny]

This seems to be the source of new test failure on 32-bit x86:

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AlignOfTest
[ RUN      ] AlignOfTest.BasicAlignedArray
/var/tmp/portage/sys-devel/llvm-9999/work/llvm-9999/unittests/Support/AlignOfTest.cpp:151: Failure
      Expected: alignof(T<long long>)
      Which is: 4
To be equal to: alignof(AlignedCharArrayUnion<long long>)
      Which is: 8
/var/tmp/portage/sys-devel/llvm-9999/work/llvm-9999/unittests/Support/AlignOfTest.cpp:153: Failure
      Expected: alignof(T<double>)
      Which is: 4
To be equal to: alignof(AlignedCharArrayUnion<double>)
      Which is: 8
[  FAILED  ] AlignOfTest.BasicAlignedArray (1 ms)

It should be noted that this specific commit fails due to assertion:

/home/mgorny/llvm/unittests/Support/AlignOfTest.cpp:110:1: error: static assertion failed: Statically-computed alignment must be right
 static_assert(alignof(U) == alignof(uint64_t), "Statically-computed alignment must be right");
 ^

However, rL291515 (@rnk) following it builds fine and has this test failure.

llvm/trunk/include/llvm/Support/AlignOf.h

 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(32)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(64)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(128)
 
 #undef LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT
 
 #endif // _MSC_VER
 
+// This code implements the equivalent of std::aligned_union from C++11.
+// That is supported by Visual Studio 2015 and GCC 5.1.
+// Once these are the baselines for LLVM, we can use std::aligned_union instead.
+
 namespace detail {
-template <typename T1,
-          typename T2 = char, typename T3 = char, typename T4 = char,
-          typename T5 = char, typename T6 = char, typename T7 = char,
-          typename T8 = char, typename T9 = char, typename T10 = char>
-class AlignerImpl {
-  T1 t1; T2 t2; T3 t3; T4 t4; T5 t5; T6 t6; T7 t7; T8 t8; T9 t9; T10 t10;
+template <typename... Ts> class AlignerImpl;
 
+template <typename T1, typename... Ts>
+class AlignerImpl<T1, Ts...> : AlignerImpl<Ts...> {
+  T1 t;
   AlignerImpl() = delete;
 };
 
-template <typename T1,
-          typename T2 = char, typename T3 = char, typename T4 = char,
-          typename T5 = char, typename T6 = char, typename T7 = char,
-          typename T8 = char, typename T9 = char, typename T10 = char>
-union SizerImpl {
-  char arr1[sizeof(T1)], arr2[sizeof(T2)], arr3[sizeof(T3)], arr4[sizeof(T4)],
-       arr5[sizeof(T5)], arr6[sizeof(T6)], arr7[sizeof(T7)], arr8[sizeof(T8)],
-       arr9[sizeof(T9)], arr10[sizeof(T10)];
-};
+template <> class AlignerImpl<> { AlignerImpl() = delete; };
+
+template <typename T1> constexpr size_t sizer() { return sizeof(T1); }
+
+template <typename T1, typename T2, typename... Ts> constexpr size_t sizer() {
+  return (sizeof(T1) > sizer<T2, Ts...>()) ? sizeof(T1) : sizer<T2, Ts...>();
+}
 } // end namespace detail
 
 /// \brief This union template exposes a suitably aligned and sized character
-/// array member which can hold elements of any of up to ten types.
+/// array member which can hold elements of any of a number of types.
 ///
 /// These types may be arrays, structs, or any other types. The goal is to
 /// expose a char array buffer member which can be used as suitable storage for
-/// a placement new of any of these types. Support for more than ten types can
-/// be added at the cost of more boilerplate.
-template <typename T1,
-          typename T2 = char, typename T3 = char, typename T4 = char,
-          typename T5 = char, typename T6 = char, typename T7 = char,
-          typename T8 = char, typename T9 = char, typename T10 = char>
-struct AlignedCharArrayUnion : llvm::AlignedCharArray<
-    alignof(llvm::detail::AlignerImpl<T1, T2, T3, T4, T5,
-                                      T6, T7, T8, T9, T10>),
-    sizeof(::llvm::detail::SizerImpl<T1, T2, T3, T4, T5,
-                                     T6, T7, T8, T9, T10>)> {
-};
+/// a placement new of any of these types.
+template <typename... Ts>
+struct AlignedCharArrayUnion
+    : llvm::AlignedCharArray<alignof(llvm::detail::AlignerImpl<Ts...>),
+                             detail::sizer<Ts...>()> {};
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_ALIGNOF_H

llvm/trunk/unittests/Support/AlignOfTest.cpp

 V4::~V4() {}
 V5::~V5() {}
 V6::~V6() {}
 V7::~V7() {}
 V8::~V8() {}
 
 template <typename M> struct T { M m; };
 
+typedef uint8_t t1;
+typedef uint16_t t2;
+typedef uint32_t t3;
+typedef uint64_t t4;
+typedef int8_t t5;
+typedef int16_t t6;
+typedef int32_t t7;
+typedef int64_t t8;
+typedef struct { uint8_t bytes[16]; } t9;
+typedef struct { uint16_t words[16]; } t10;
+typedef struct { uint32_t words[16]; } t11;
+typedef struct { uint64_t words[16]; } t12;
+
+typedef AlignedCharArrayUnion<t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, t11, t12> U;
+
+static_assert(sizeof(U) == sizeof(uint64_t[16]), "Statically-computed size must be right");
+static_assert(alignof(U) == alignof(uint64_t), "Statically-computed alignment must be right");
+
 TEST(AlignOfTest, BasicAlignedArray) {
   EXPECT_LE(1u, alignof(AlignedCharArrayUnion<A1>));
   EXPECT_LE(2u, alignof(AlignedCharArrayUnion<A2>));
   EXPECT_LE(4u, alignof(AlignedCharArrayUnion<A4>));
   EXPECT_LE(8u, alignof(AlignedCharArrayUnion<A8>));
 
   EXPECT_LE(1u, sizeof(AlignedCharArrayUnion<A1>));
   EXPECT_LE(2u, sizeof(AlignedCharArrayUnion<A2>));