[libcxx] String format class marked as packed

[stefanp]

Summary

This patch marks the class _Flags as packed because the design assumes that it
is packed and a number of tests also assume that it is packed. However on AIX
the class is not packed unless it is marked as such.

[hubert.reinterpretcast]

libcxx/include/__format/parser_std_format_spec.h

111

Use double-underscore form to avoid macro problems.

Actually, are we assuming GNU attribute support in general "everywhere"?

[Mordante]

Thanks for fixing these build issues!

libcxx/include/__format/parser_std_format_spec.h

111

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.

[hubert.reinterpretcast]

libcxx/include/__format/parser_std_format_spec.h

111

Is there something like godbolt where I can check the layout on AIX online?

Yes: godbolt

https://godbolt.org/z/9j49baqsM

[Mordante]

libcxx/include/__format/parser_std_format_spec.h

111

Thanks I wasn't aware godbolt also has AIX support. Good to know for the future.

[stefanp]

I've updated the packed attribute and added the under-under for it.

In terms of using the attribute I know that other parts of libcxx already
use it so I figured it would be safe to use as it would be nothing new.

[hubert.reinterpretcast]

LGTM from the AIX viewpoint. The resulting layout is similar to what would happen (even without the attribute) on Linux.
Please wait for the libc++ group approval.

[Mordante]

LGTM. When I make the changes to this type I might make the __packed__ conditional. But that depends on how it looks in the final version. At least now I know I can check AIX on godbolt.

[ldionne]

You have a typo in your commit message (becuase). Generally LGTM with a small style comment. I'd also like to better understand the motivation before shipping this: can someone explain why/how the tests currently depend on the class being packed?

libcxx/include/__format/parser_std_format_spec.h

111

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.

[stefanp]

In D119567#3321211, @ldionne wrote:

You have a typo in your commit message (becuase). Generally LGTM with a small style comment. I'd also like to better understand the motivation before shipping this: can someone explain why/how the tests currently depend on the class being packed?

Thank you for catching that. I'll fix the typo before I commit.

I can explain why the tests depend on the class being packed. All of the tests check the size of the class Parser with an assert like this one:

static_assert(sizeof(Parser<char>) == 2 * sizeof(uint32_t));

The class _Flags is part of the Parser class and so if _Flags is not packed then Parser will be larger and the assert will fail.

The tests and asserts were added when the original Formatter implementations were done. (See 7fb9f99f3bb645337b4f4e6a2a3515219be82011 from patch https://reviews.llvm.org/D103670) . I didn't add the original implementation but I think the reason for these asserts was that the author wanted to make sure that the data structures where small.

[stefanp]

Moved the attribute to the top of the class.
Typo in the comment.

[joerg]

libcxx/include/__format/parser_std_format_spec.h

111

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.

[ldionne]

LGTM since this looks good to @Mordante, who wrote this code in the first place. I'm curious to better understand what @joerg 's comment is, though.

libcxx/include/__format/parser_std_format_spec.h

111

Can you elaborate?

[• Quuxplusone]

libcxx/include/__format/parser_std_format_spec.h

55

If I ran the zoo, I'd make this struct __attribute__((__packed__)) only on AIX, and leave it "normal" everywhere else. I'm worried that a packed struct might behave observably differently on some non-AIX platform and have some effect that we didn't expect. But I'm not aware of any concrete problem with how you've got it now.

Pre-existing nit: I would also move lines 83–87 down next to line 110 to keep all the members together.

[joerg]

libcxx/include/__format/parser_std_format_spec.h

111

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.

[Mordante]

libcxx/include/__format/parser_std_format_spec.h

55

As commented below this _Flags class will be changed, so for now I don't mind the unconditional __packed__. I discovered some issues with the current design:

• A formatter is-a parser, when changing it to has-a parser it leads to a lot less code duplication in the binary. On an old branch I can reduce 12KB of object size. (I believe I can get an even better size reduction.)
• The is-a design makes it hard/impossible to make the format member function const qualified. This will become mandatory after LWG3636 you filed is accepted. I also believe this change makes it easier to implement P2286 Formatting ranges, which AFAIK is still targetting C++23.

In this new design I will test whether __packed__ is still needed on AIX or not. (The goal is to keep the struct small so it can be passed in registers.) I learned godbolt has AIX support so that makes testing a lot easier.

[hubert.reinterpretcast]

libcxx/include/__format/parser_std_format_spec.h

111

@ldionne, what are your thoughts on creating such a macro? Something like __LIBCPP_PACKED(1).

[stefanp]

Updated to use _Pragma pack instead of using the attribute.
Also, made the change only for AIX.

[hubert.reinterpretcast]

I think this version addresses all of the comments made. The parameterized form of the macro can be added when needed. Confirming LGTM.

libcxx/include/__config

1444 ↗

(On Diff #410029)

Yes, I agree this is the better form (the effect of the packing is controlled by the argument, unlike the attribute, which accepts no arguments). The effect on the alignment of the members is mitigated with this form (if the parameter is not 1).

If we need this parameterized in the future, the following works (even if pack is defined as a macro):

#define _LIBCPP_PACKED_FOR_AIX_IMPL(X) _Pragma( # X )
#define _LIBCPP_PACKED_FOR_AIX(X) _LIBCPP_PACKED_FOR_AIX_IMPL( pack( X ) )

[ldionne]

libcxx/include/__format/parser_std_format_spec.h

111

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.

libcxx/include/__format/parser_std_format_spec.h

  *
  * 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.
  */
-class _LIBCPP_TYPE_VIS _Flags {
+class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags {
 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 {
     __scientific_upper_case,
     __fixed_lower_case,
     __fixed_upper_case,
     __general_lower_case,
     __general_upper_case
   };
 
   _Type __type{_Type::__default};
 };
 
 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;

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_bool.pass.cpp

 //===----------------------------------------------------------------------===//
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-format
-// XFAIL: LIBCXX-AIX-FIXME
 
 // <format>
 
 // Tests the parsing of the format string as specified in [format.string.std].
 // It validates whether the std-format-spec is valid for a boolean type.
 
 #include <format>
 #include <cassert>

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_char.pass.cpp

 //===----------------------------------------------------------------------===//
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-format
-// XFAIL: LIBCXX-AIX-FIXME
 
 // <format>
 
 // Tests the parsing of the format string as specified in [format.string.std].
 // It validates whether the std-format-spec is valid for a char type.
 
 #include <format>
 #include <cassert>

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_integer.pass.cpp

 //===----------------------------------------------------------------------===//
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-format
-// XFAIL: LIBCXX-AIX-FIXME
 
 // <format>
 
 // Tests the parsing of the format string as specified in [format.string.std].
 // It validates whether the std-format-spec is valid for an integer type.
 
 #include <format>
 #include <cassert>

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_string.pass.cpp

 //===----------------------------------------------------------------------===//
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-format
-// XFAIL: LIBCXX-AIX-FIXME
 
 // <format>
 
 // Tests the parsing of the format string as specified in [format.string.std].
 // It validates whether the std-format-spec is valid for a string type.
 
 #include <format>
 #include <cassert>