[NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[jfb]

Summary

I removed all uses of AlignedCharArray since the minimum MSVC version can handle alignas on char arrays correctly. We can therefore remove AlignedCharArray.

This patch also updates AlignedCharArrayUnion to use C++11.

[jfb]

This is the main event: https://reviews.llvm.org/D65249#change-IWk6CtRl45h6
The rest is side-show.

[rnk]

I still think this concern applies:
https://reviews.llvm.org/D64417#1576675

I'm not sure how to track down an 19.11 release to test if we can pass std::aligned_storage values through function calls on x86. We might be better off raising the minimum to 19.14, which is a more recent MSVC release in the 2017 track. I don't think it's too much to ask developers to use the most recent version of the 2017 compiler, they won't have to change IDEs.

[jfb]

In D65249#1603133, @rnk wrote:

I still think this concern applies:
https://reviews.llvm.org/D64417#1576675

I'm not sure how to track down an 19.11 release to test if we can pass std::aligned_storage values through function calls on x86. We might be better off raising the minimum to 19.14, which is a more recent MSVC release in the 2017 track. I don't think it's too much to ask developers to use the most recent version of the 2017 compiler, they won't have to change IDEs.

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

If it does then indeed we should raise the minimum version, I'll send an RFC.

[BillyONeal]

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

aligned_storage is still capped at what the library can fake with unions. It would be an ABI break to change it to use alignas, so things where the x86 stack temporarily breaks T's usual alignment rules will affect it (and for all releases using VS2015's ABI). We recommend customers use alignas(T) unsigned char space[sizeof(T)] if they can't tolerate such limitations.

[jfb]

In D65249#1603278, @BillyONeal wrote:

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

aligned_storage is still capped at what the library can fake with unions. It would be an ABI break to change it to use alignas, so things where the x86 stack temporarily breaks T's usual alignment rules will affect it (and for all releases using VS2015's ABI). We recommend customers use alignas(T) unsigned char space[sizeof(T)] if they can't tolerate such limitations.

Thanks!

@rnk: how about I add a bit of code that wraps aligned_storage on all platforms except MSVC (where I'd implement it as Billy suggests). That would mean updating all the uses of aligned_storage to this LLVM one, but that's not a big deal.

[BillyONeal]

In D65249#1603335, @jfb wrote:

@rnk: how about I add a bit of code that wraps aligned_storage on all platforms except MSVC (where I'd implement it as Billy suggests). That would mean updating all the uses of aligned_storage to this LLVM one, but that's not a big deal.

Why not just always use the alignas version? Or even better, not involve a library for something you can say in the core language at all?

[BillyONeal]

(In fact I observe many patterns in this review like:

enum { Foo = alignof(void*); }
aligned_storage_t<1234, Foo> x;

and then a bunch of casting to treat it as a char buffer; if it was just born as a char buffer you can remove both the casts and the enum hack:

alignas(void*) char x[1234];

[jfb]

Significantly simplify the patch.

[jfb]

In D65249#1603431, @BillyONeal wrote:

(In fact I observe many patterns in this review like:

enum { Foo = alignof(void*); }
aligned_storage_t<1234, Foo> x;

and then a bunch of casting to treat it as a char buffer; if it was just born as a char buffer you can remove both the casts and the enum hack:

alignas(void*) char x[1234];

Good point. I went ahead and did that. The patch is now quite trivial :)

[rnk]

I have concerns that some of the patches that you landed prior to this will cause issues with old versions of MSVC, but in isolation, this is fine, and if anyone complains, we can address it on a case-by-case basis. lgtm

[jfb]

In D65249#1605523, @rnk wrote:

I have concerns that some of the patches that you landed prior to this will cause issues with old versions of MSVC, but in isolation, this is fine, and if anyone complains, we can address it on a case-by-case basis. lgtm

IIUC Billy correctly, the issue was just with aligned_storage, but if you roll your own (as I have) we're fine. If I'm wrong, these tests will fail on MSVC bots :)

[jfb]

https://reviews.llvm.org/rL367282

[mgrang]

Hi @jfb @rnk This patch results in a compiler crash when building a simple C program on a Windows X86 Debug build. I have filed this issue to track it.

llvm/include/llvm/Support/AlignOf.h

 //===--- AlignOf.h - Portable calculation of type alignment -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// This file defines the AlignedCharArray and AlignedCharArrayUnion classes.
+// This file defines the AlignedCharArrayUnion class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_ALIGNOF_H
 #define LLVM_SUPPORT_ALIGNOF_H
 
 #include "llvm/Support/Compiler.h"
 #include <cstddef>
 
 namespace llvm {
 
-/// \struct AlignedCharArray
-/// Helper for building an aligned character array type.
-///
-/// This template is used to explicitly build up a collection of aligned
-/// character array types. We have to build these up using a macro and explicit
-/// specialization to cope with MSVC (at least till 2015) where only an
-/// integer literal can be used to specify an alignment constraint. Once built
-/// up here, we can then begin to indirect between these using normal C++
-/// template parameters.
-
-// MSVC requires special handling here.
-#ifndef _MSC_VER
-
-template<std::size_t Alignment, std::size_t Size>
-struct AlignedCharArray {
-  alignas(Alignment) char buffer[Size];
-};
-
-#else // _MSC_VER
-
-/// Create a type with an aligned char buffer.
-template<std::size_t Alignment, std::size_t Size>
-struct AlignedCharArray;
-
-// We provide special variations of this template for the most common
-// alignments because __declspec(align(...)) doesn't actually work when it is
-// a member of a by-value function argument in MSVC, even if the alignment
-// request is something reasonably like 8-byte or 16-byte. Note that we can't
-// even include the declspec with the union that forces the alignment because
-// MSVC warns on the existence of the declspec despite the union member forcing
-// proper alignment.
-
-template<std::size_t Size>
-struct AlignedCharArray<1, Size> {
-  union {
-    char aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<2, Size> {
-  union {
-    short aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<4, Size> {
-  union {
-    int aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<8, Size> {
-  union {
-    double aligned;
-    char buffer[Size];
-  };
-};
-
-
-// The rest of these are provided with a __declspec(align(...)) and we simply
-// can't pass them by-value as function arguments on MSVC.
+namespace detail {
 
-#define LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(x) \
-  template<std::size_t Size> \
-  struct AlignedCharArray<x, Size> { \
-    __declspec(align(x)) char buffer[Size]; \
-  };
+template <typename T, typename... Ts> class AlignerImpl {
+  T t;
+  AlignerImpl<Ts...> rest;
+  AlignerImpl() = delete;
 
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(16)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(32)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(64)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(128)
-
-#undef LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT
-
-#endif // _MSC_VER
-
-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 T> class AlignerImpl<T> {
+  T 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 <typename T, typename... Ts> union SizerImpl {
+  char arr[sizeof(T)];
+  SizerImpl<Ts...> rest;
 };
+
+template <typename T> union SizerImpl<T> { char arr[sizeof(T)]; };
 } // end namespace detail
 
-/// This union template exposes a suitably aligned and sized character
-/// array member which can hold elements of any of up to ten types.
+/// A suitably aligned and sized character array member which can hold elements
+/// of any type.
 ///
-/// 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>)> {
+/// These types may be arrays, structs, or any other types. This exposes a
+/// `buffer` member which can be used as suitable storage for a placement new of
+/// any of these types.
+template <typename T, typename... Ts> struct AlignedCharArrayUnion {
+  alignas(::llvm::detail::AlignerImpl<T, Ts...>) char buffer[sizeof(
+      llvm::detail::SizerImpl<T, Ts...>)];
 };
+
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_ALIGNOF_H

llvm/unittests/Support/AlignOfTest.cpp

#endif
   EXPECT_EQ(sizeof(V6), sizeof(AlignedCharArrayUnion<V6>));
   EXPECT_EQ(sizeof(V7), sizeof(AlignedCharArrayUnion<V7>));
 
   // Some versions of MSVC also get this wrong. The failure again appears to be
   // benign: sizeof(V8) is only 52 bytes, but our array reserves 56.
 #ifndef _MSC_VER
   EXPECT_EQ(sizeof(V8), sizeof(AlignedCharArrayUnion<V8>));
 #endif
-
-  EXPECT_EQ(1u, (alignof(AlignedCharArray<1, 1>)));
-  EXPECT_EQ(2u, (alignof(AlignedCharArray<2, 1>)));
-  EXPECT_EQ(4u, (alignof(AlignedCharArray<4, 1>)));
-  EXPECT_EQ(8u, (alignof(AlignedCharArray<8, 1>)));
-  EXPECT_EQ(16u, (alignof(AlignedCharArray<16, 1>)));
-
-  EXPECT_EQ(1u, sizeof(AlignedCharArray<1, 1>));
-  EXPECT_EQ(7u, sizeof(AlignedCharArray<1, 7>));
-  EXPECT_EQ(2u, sizeof(AlignedCharArray<2, 2>));
-  EXPECT_EQ(16u, sizeof(AlignedCharArray<2, 16>));
 }
 } // end anonymous namespace