[PATCH] D119567: [libcxx] String format class marked as packed

[Joerg Sonnenberger via Phabricator via llvm-commits]

joerg added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:111
   _Type __type{_Type::__default};
-};
+} __attribute__((packed));
 
----------------
ldionne wrote:
> Mordante wrote:
> > hubert.reinterpretcast wrote:
> > > Mordante wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > Use double-underscore form to avoid macro problems.
> > > > > 
> > > > > Actually, are we assuming GNU attribute support in general "everywhere"?
> > > > Is there something like godbolt where I can check the layout on AIX online?
> > > > I've been working on some changes regarding this class. In that new version unconditionally pack the struct on all platforms might lead to sub-optimal code. For the current code I don't see an issue with the packing.
> > > > 
> > > > > Actually, are we assuming GNU attribute support in general "everywhere"?
> > > > 
> > > > I theory yes. The pre-commit CI tests all supported platforms. So when the CI passes it works. (The ASAN errors are unrelated.) But I think it would be cleaner to make the packing conditional.
> > > > 
> > > > 
> > > > Is there something like godbolt where I can check the layout on AIX online?
> > > 
> > > Yes: godbolt
> > > 
> > > https://godbolt.org/z/9j49baqsM
> > Thanks I wasn't aware godbolt also has AIX support. Good to know for the future.
> Can't we put that attribute at the beginning of the class declaration, i.e. `class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags`?
> 
> This whole time I thought the attribute applied to something different because of the (IMO) unusual placement.
No, we are consciously not using naked GNU attributes in the headers. I'd also ask for an explicit alignment, even if it is 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119567