[libc++] Simplify how __format_spec::_Flags is packed on AIX

[ldionne]

Summary

I don't think we really need to introduce a mechanism involving pragma
push/pop for just that tiny workaround. I'd rather keep the workaround
localized than pretend it's a general thing and complicate our __config
for something that we'll only ever use in one place.

[• Quuxplusone]

libcxx/include/__format/parser_std_format_spec.h

49

Hm, counter-argument: unless we're going to #undef this at the end of the header, it feels like this is something generic that might get copy-pasted around, and would have to move into a shared header at that point to avoid multiple definitions. If we're going to localize it into this one file, personally I'd prefer just

#ifdef _AIX
class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags {
#else
class _LIBCPP_TYPE_VIS _Flags {
#endif // _AIX

(But I don't care much.)

[ldionne]

Address comment

libcxx/include/__format/parser_std_format_spec.h

49

Agreed, I like yours better.

@hubert.reinterpretcast I think it would be necessary to know whether this will have to be repeated in many places. I actually don't quite understand what makes this struct so special -- why the compiler packs it on all platforms except AIX. CC @Mordante

[hubert.reinterpretcast]

libcxx/include/__format/parser_std_format_spec.h

49

@ldionne, any struct having bitfields on AIX would have at least size 4 unless if marked as packed.

This pattern has shown up twice in the LLVM source:

./include/llvm/CodeGen/SelectionDAGNodes.h-464-
./include/llvm/CodeGen/SelectionDAGNodes.h-465-#if defined(_AIX) && (!defined(__GNUC__) || defined(__clang__))
./include/llvm/CodeGen/SelectionDAGNodes.h-466-// Except for GCC; by default, AIX compilers store bit-fields in 4-byte words
./include/llvm/CodeGen/SelectionDAGNodes.h-467-// and give the `pack` pragma push semantics.
./include/llvm/CodeGen/SelectionDAGNodes.h:468:#define BEGIN_TWO_BYTE_PACK() _Pragma("pack(2)")
./include/llvm/CodeGen/SelectionDAGNodes.h:469:#define END_TWO_BYTE_PACK() _Pragma("pack(pop)")
./include/llvm/CodeGen/SelectionDAGNodes.h-470-#else
./include/llvm/CodeGen/SelectionDAGNodes.h-471-#define BEGIN_TWO_BYTE_PACK()
./include/llvm/CodeGen/SelectionDAGNodes.h-472-#define END_TWO_BYTE_PACK()
./include/llvm/CodeGen/SelectionDAGNodes.h-473-#endif
--
./include/llvm/IR/BasicBlock.h-515-private:
./include/llvm/IR/BasicBlock.h-516-#if defined(_AIX) && (!defined(__GNUC__) || defined(__clang__))
./include/llvm/IR/BasicBlock.h-517-// Except for GCC; by default, AIX compilers store bit-fields in 4-byte words
./include/llvm/IR/BasicBlock.h-518-// and give the `pack` pragma push semantics.
./include/llvm/IR/BasicBlock.h:519:#define BEGIN_TWO_BYTE_PACK() _Pragma("pack(2)")
./include/llvm/IR/BasicBlock.h:520:#define END_TWO_BYTE_PACK() _Pragma("pack(pop)")
./include/llvm/IR/BasicBlock.h-521-#else
./include/llvm/IR/BasicBlock.h-522-#define BEGIN_TWO_BYTE_PACK()
./include/llvm/IR/BasicBlock.h-523-#define END_TWO_BYTE_PACK()
./include/llvm/IR/BasicBlock.h-524-#endif

The pragma form offers better semantics than the attribute: The attribute form will reduce alignment of other members beyond what is necessary. The pragma form only reduces to the argument value.

[ldionne]

libcxx/include/__format/parser_std_format_spec.h

49

Alright. I'm not a big fan of this workaround but I'll abandon this patch in case we need to use it elsewhere. TBH, it seems weird for the compiler to have that behavior, but only on AIX.

[Mordante]

libcxx/include/__format/parser_std_format_spec.h

49

@ldionne, any struct having bitfields on AIX would have at least size 4 unless if marked as packed.

Interesting to know, thanks for this information.
The new version of _Flags I'm working on has 4-bytes of flags. So that means this work-around should really be temporary.

libcxx/include/__config

 #  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING(GCC diagnostic ignored str))
 #else
 #  define _LIBCPP_DIAGNOSTIC_PUSH
 #  define _LIBCPP_DIAGNOSTIC_POP
 #  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
 #  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
 #endif
 
-#if defined(_AIX) && !defined(_LIBCPP_COMPILER_GCC)
-#  define _LIBCPP_PACKED_BYTE_FOR_AIX _Pragma("pack(1)")
-#  define _LIBCPP_PACKED_BYTE_FOR_AIX_END _Pragma("pack(pop)")
-#else
-#  define _LIBCPP_PACKED_BYTE_FOR_AIX      /* empty */
-#  define _LIBCPP_PACKED_BYTE_FOR_AIX_END  /* empty */
-#endif
-
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/include/__format/parser_std_format_spec.h

 # if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 namespace __format_spec {
 
 /**
  * Contains the flags for the std-format-spec.
  *
  * Some format-options can only be used for specific C++ types and may depend on
  * the selected format-type.
  * * The C++type filtering can be done using the proper policies for
  *   @ref __parser_std.
  * * The format-type filtering needs to be done post parsing in the parser
  *   derived from @ref __parser_std.
  */
-_LIBCPP_PACKED_BYTE_FOR_AIX
-class _LIBCPP_TYPE_VIS _Flags {
+#if defined(_AIX)
+class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags
+#else
+class _LIBCPP_TYPE_VIS _Flags
+#endif
+{
 public:
   enum class _LIBCPP_ENUM_VIS _Alignment : uint8_t {
     /**
      * No alignment is set in the format string.
      *
      * Zero-padding is ignored when an alignment is selected.
      * The default alignment depends on the selected format-type.
      */
enum class _LIBCPP_ENUM_VIS _Type : uint8_t {
     __fixed_lower_case,
     __fixed_upper_case,
     __general_lower_case,
     __general_upper_case
   };
 
   _Type __type{_Type::__default};
 };
-_LIBCPP_PACKED_BYTE_FOR_AIX_END
 
 namespace __detail {
 template <class _CharT>
 _LIBCPP_HIDE_FROM_ABI constexpr bool
 __parse_alignment(_CharT __c, _Flags& __flags) noexcept {
   switch (__c) {
   case _CharT('<'):
     __flags.__alignment = _Flags::_Alignment::__left;