[libcxx-commits] [PATCH] D121009: [libc++] Simplify how __format_spec::_Flags is packed on AIX

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:49
+# define _LIBCPP_PACK_FLAGS_ON_AIX
+#endif
+
----------------
ldionne wrote:
> hubert.reinterpretcast wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > 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.)
> > > 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 
> > @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.
> 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.
> @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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121009/new/

https://reviews.llvm.org/D121009