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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Mar 4 09:36:12 PST 2022


ldionne added inline comments.
Herald added a project: All.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:111
   _Type __type{_Type::__default};
-};
+} __attribute__((packed));
 
----------------
hubert.reinterpretcast wrote:
> joerg wrote:
> > ldionne wrote:
> > > joerg wrote:
> > > > 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.
> > > Can you elaborate?
> > Like __LIBCPP_HIDE_FROM_ABI etc., they are encapsulated and restricted to platforms that are known to support them.
> > 
> > I consider a naked packed attribute generally a bug, see also the clang warning around members of packed structures. This isn't a huge issue here as everything is uint8_t or smaller, but better to be consistent.
> @ldionne, what are your thoughts on creating such a macro? Something like `__LIBCPP_PACKED(1)`.
Hmm, I missed this part of the review, but TBH I think the solution we ended with is kind of over-engineered. Having to push/pop a pragma just for this seems overkill.

Also, @joerg , we support a limited number of compilers, and we do frequently use things (e.g. builtins) without creating an unnecessary "abstraction layer" on top by e.g. creating a `_LIBCPP_FOO` macro.

I spun up D121009, let's move the discussion there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119567



More information about the libcxx-commits mailing list