[libc++] Use bit field for checking if string is in long or short mode

[philnik]

Summary

This makes the code a bit simpler and (I think) removes the undefined behaviour from the normal string layout.

[philnik]

• Fix pretty printers

[Mordante]

In general I like this direction, but I'm somewhat concerned regarding ABI breaks.

libcxx/include/string

681

Do we have a test that validates the size of this struct?

On some platforms (IIRC AIX) the members __cap_ and __is_long_ won't be stored in one size_type since they have different types.

681

Did you verify this isn't an ABI break when libc++14 and libc++15 compiled code is mixed?
We should make sure that doesn't break an big and little endian.

[Mordante]

libcxx/include/string

696

Since you now understand how this works, it would be nice to add some documentation.

703

Is this a copy paste?

1486

Mainly curious, but why not a _LIBCPP_ASSERT?

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Does this also break the LLDB pretty printer?

[philnik]

libcxx/include/string

681

https://godbolt.org/z/axTMxqaj3 is the code I used to verify that everything is OK. I think this should be good enough to say that they are equivalent. Do you know if there is any documentation about the bit field layout on different platforms?

703

No, depending on the string layout the factors have to be reversed. It's essentially: If you use the LSB for is_long you have to multiply by two, otherwise keep the same number.

1486

This ensures optimal code generation. GCC doesn't have __builtin_assume. This is a workaround.

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Probably. Would be nice to have a test runner for that.

[Mordante]

libcxx/include/string

681

Good to know.
BTW it's not AIX but win32 where the different types are an issue. (AIX had something else with bitfields)
https://godbolt.org/z/n3zr5v4sr The different types make the struct 32 bytes on Windows.

I don't know where the information regarding the bit field layout can be found.
(I guess Google or the LLVM sources can help out.)

703

I see, I missed the #else on line 701.

1486

From a maintainability PoV I like _LIBCPP_ASSERT better. Would it be an option to adjust _LIBCPP_ASSERT? the "generate" this code when using GCC?

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

I already planned to look into that, D97044#3440904 ;-)

[ldionne]

libcxx/include/string

696

Indeed -- it would be awesome if you could add some documentation explaining how these representations work, and that may also help in catching potential issues with this patch.

[philnik]

libcxx/include/string

1486

I agree that _LIBCPP_ASSERT would be nicer to read. The problem is that using this trick for _LIBCPP_ASSERT could lead to extra code generation in more complex scenarios. We would have to introduce something else like _LIBCPP_ASSUME, which should only be used for trivial cases. But for two instances I don't think this is justified.

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Do you know where I would have to look to know what the LLDB pretty printers do?

[philnik]

• Address comments; Try to fix CI

[ldionne]

I really like this cleanup, I think it makes it much easier to understand what's going on. I do have some comments, but this is looking pretty good.

libcxx/include/string

695–700

IMO this adds a bit of clarity.

1485

I think I understand that you based the bound on the number representable by the __size_ bitfield. We could also establish it based on the maximum capacity of the small string buffer by using __s >= __min_cap instead. (>= because of the null terminator).

1486

Can you try again with _LIBCPP_ASSERT by making __libcpp_assertion_handler noreturn? This is how GCC implements their own assert(...), so presumably there might be some optimization opportunities for them there. I think this would be much better than using unreachable().

Edit: Yuck, this indeed won't help when assertions are disabled on GCC. However, looking at the codegen, it appears that the only difference is that the compiler adds a single instruction to ensure that __s <= 127. Unless you can show us a case where there is a significant difference, I think the way to go is to use _LIBCPP_ASSERT and for GCC to be able to pick it up.

1493–1494

Same comment about using _LIBCPP_ASSERT -- unless we have a very convincing reason not to use it, let's use it.

3215

After discussing it, I think we agree this is a bit easier to digest.

3219

I see the CI is failing on backdeployment macOS arm64. You just explained to me that this was caused by a bug where we used to return an incorrect max_size() in Big Endian -- the test would expect that we throw length_error, but we don't and instead we try allocating, which leads to bad_alloc instead. This patch fixes that, but since max_size() has been inlined into the dylib, the test will fail in backdeployment.

Since it seems that we are fixing a bug as part of this patch, let's add a regression test for it. We can then mark that test (and the failing over_max_size.pass.cpp) as // XFAIL: use_system_cxx_lib && target=arm64{{.+}}-apple-macosx{{11.0|12.0}}, with an appropriate comment.

libcxx/utils/gdb/libcxx/printers.py

1 ↗

(On Diff #422330)

@saugustine The pretty-printer tests have been disabled on recent Clang versions since D118067. It would be nice to fix them.

[philnik]

libcxx/include/string

3219

I don't think we have to mark the new test as XFAIL because it doesn't use the dylib version.

[philnik]

• Address comments

[philnik]

• Use the correct max_size

[ldionne]

LGTM with fixed test and passing CI.

Note that this is really tricky. The code looks correct upon inspection, and I think we should catch pretty much any issue via CI. However, if weird stuff starts happening with std::string in the upcoming few weeks (when I'm OOO), this should be the number one suspect, and it should be considered fine to revert this in case we suspect std::string is broken.

libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp

10 ↗

(On Diff #422651)

Please add a short comment explaining what you're testing in this test. We already have a max_size.pass.cpp file in test/std, so how is this one different? The comment should address that.

25–27 ↗

(On Diff #422651)

Instead of those #ifdefs, I think we should instead be checking for specific platforms/architectures. Otherwise, this test informs us less about the behavior that we expect on specific platforms, and more about what the library itself does. So for example, if we changed the library in a way that broke behavior on a given platform, this test could keep passing as long as we were consistent w.r.t. _LIBCPP_BIG_ENDIAN and _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT.

So instead, I suggest going for something like

#if defined(__APPLE__) && defined(__aarch64__)
// test behavior for alternate string layout
#elif ...
// test behavior for normal string layout
#endif

I don't think it should be *too* hard to come up with those #ifdefs to satisfy the CI.

libcxx/test/std/strings/basic.string/string.capacity/over_max_size.pass.cpp

11 ↗

(On Diff #422651)

Please add a comment explaining why you're adding this one. Something like

// Prior to http://llvm.org/D123580, there was a bug with how the max_size()
// was calculated. That was inlined into some functions in the dylib, which leads
// to failures when running this test against an older system dylib.
// XFAIL: use_system_cxx_lib && target=arm64{{.+}}-apple-macosx{{11.0|12.0}}

I don't know why the above XFAIL line doesn't have a comment, but one should have been added when we XFAILed it. Of course you don't need to add one, but that's the spirit.

[Mordante]

LGTM, with some minor remarks. I think it would be good to wait for feedback from the LLDB devs to validate whether this breaks the data formatters.

libcxx/include/string

707

Thanks for the description!

1485

libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp

20 ↗

(On Diff #422651)

For completeness please add tests for std::wstring and std::u8string, both properly guarded.
Note that the sizeof(wchar_t) can be 16 or 32-bit.

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Unfortunately no. @jingham seems to be the Data formatter code owner.

[jgorbe]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

There was a recent lldb change fixing prettyprinters after a similar change to string: https://github.com/llvm/llvm-project/commit/45428412fd7c9900d3d6ac9803aa7dcf6adfa6fe

If the gdb prettyprinter needed fixing for this change, chances are that lldb will need a similar update too.

[philnik]

• Address comments

[philnik]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Could someone from #lldb help me figure out what to change in the pretty printers? I looked at the file, but I don't really understand how it works and TBH I don't really feel like spending a lot of time figuring it out. If nobody says anything I'll land this in a week.

As a side note: it would be really nice if there were a few more comments inside LibCxx.cpp to explain what happens there. That would make fixing the pretty printer a lot easier. The code is probably not very hard (at least it doesn't look like it), but I am looking for a few things that I can't find and I have no idea what some of the things mean.

[philnik]

• Fix CI

[philnik]

• Add Windows

[dblaikie]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Looks like something around https://github.com/llvm/llvm-project/blob/2e6ac54cf48aa04f7b05c382c33135b16d3f01ea/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp#L597 (& the similar masking in the else block a few lines down) - I guess a specific lookup for the new field would be needed, rather than the bitmasking.

[philnik]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Yes, but what do the numbers in size_mode_locations mean? Why is there no checking if it's big or little endian? Is it unsupported maybe? Does it work because of something else? Is there a reason that g_data_name exists instead of comparing directly? Should I add another layout style or should I just update the code for the new layout?
I don't know anything about the LLDB codebase, so I don't understand the code and I don't know how I should change it.

[philnik]

• Try to fix Windows

[labath]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

I don't think there's been any official policy decision either way, but historically we haven't been asking libc++ authors to update lldb pretty printers -- we would just fix them up on the lldb side when we noticed the change. The thing that has changed recently is that google started relying (and testing) more on lldb, which considerably shortened the time it takes to notice this change, and also makes it difficult for some people to make progress while we are in this state. But I don't think that means that updating the pretty printer is suddenly your responsibility.

As for your questions, I'll try to answer them as best as I can:

what do the numbers in size_mode_locations mean?

These are the indexes of fields in the string object. For some reason (unknown to me), the pretty printer uses indexes rather than field names for its work. Prompted by the previous patch, I've been trying to change that, but I haven't done it yet, as I was trying to improve the testing story (more on that later).

Why is there no checking if it's big or little endian? Is it unsupported maybe?

Most likely yes. Although most parts of lldb support big endian, I am not aware of anyone testing it on a regular basis, so it's quite likely that a lot of things are in fact broken.

Is there a reason that g_data_name exists instead of comparing directly?

LLDB uses a global string pool, so this is an attempt to reduce the number of string pool queries. The pattern is not consistently used everywhere, and overall, I wouldn't be too worried about it.

Should I add another layout style or should I just update the code for the new layout?

As the pretty printers ship with lldb, they are expected to support not just the current format, but also the past ones (within reason). This is what makes adding a new format (or just refactoring the existing code) difficult, and it's why I was trying to come up with better tests for this (it remains to be seen if I am successful).

Anyway, I think I should be able to make that pretty printer work with this patch. I should have something today or tomorrow, if you're ok with waiting that long.

[philnik]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Thanks for the answers! I think that it wouldn't be that hard for us to update the pretty printers if we have some test coverage and documentation for it. For now, is there any person/group we should ping if we suspect that we break the pretty printers? I'll wait a few days. It's not that important to land this patch soon.

[ldionne]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

The situation with pretty printers has been a source of frustration for the 4 years I've worked on libc++. I have been reaching out to various LLDB folks to get help setting up pre-commit CI for the LLDB pretty-printers in libc++'s own pipeline so that we can detect breakages in advance, but this has not been conclusive so far.

@labath @jgorbe Would you be willing to help us set up a CI job that runs the LLDB pretty printers (and only that) in our pre-commit CI infrastructure? We have the machines and all the infrastructure in place. We just need the right CMake + lit invocations. Also CC @Mordante , since he had been investigating that IIRC.

If we could notice breakages in advance, we could fix the pretty printers in the same patch where we make a change to libc++. We could call out for help from LLDB folks when needed. This would be a much smoother experience for everyone -- we would not need to revert our patches ever, and the LLDB folks would not be broken by changes that come out of the blue (as far as they are concerned).

[Mordante]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

I've indeed been working on that, but not managed yet. I can build lldb, but the dataformatter tests return UNSUPPORTED. I haven't had time to investigate this further. I hope to find some time soon. But if @labath or @jgorbe have hints how to do this I would be interested to know. Alternatively what's the best way to contact you Discourse or Discord?

[labath]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

I'm sorry about the delay. The lldb-libc++ integration is broken in several ways (different ways on different platforms), so I'm not all that surprised that it's not working for you. I don't really consider the data formatters my responsibility so I only go near them when I really have to. Still, I agree that this is not a good situation to be in, and the offer of offloading the data formatter work to the libc++ team is definitely appealing. So, I'll try to find some time to make that happen. I'm sorry I can't promise anything specific.

[Mordante]

libcxx/utils/gdb/libcxx/printers.py

192 ↗

(On Diff #422187)

Thanks for the information. The libc++ CI uses a Ubuntu 20.04 Docker image on amd64.

libcxx/include/string

 #include <__debug>
 #include <__format/enable_insertable.h>
 #include <__ios/fpos.h>
 #include <__iterator/wrap_iter.h>
 #include <__memory/allocate_at_least.h>
 #include <__utility/auto_cast.h>
 #include <__utility/move.h>
 #include <__utility/swap.h>
+#include <__utility/unreachable.h>
+#include <climits>
 #include <compare>
 #include <cstdio>  // EOF
 #include <cstdlib>
 #include <cstring>
 #include <initializer_list>
 #include <iosfwd>
 #include <iterator>
+#include <limits>
 #include <memory>
 #include <stdexcept>
 #include <string_view>
 #include <type_traits>
 #include <version>
 
 // TODO: remove these headers
 #include <__functional/binary_function.h>

 private:
 
 #ifdef _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
     struct __long
     {
         pointer   __data_;
         size_type __size_;
-        size_type __cap_;
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        bool __is_long_ : 1;
     };
 
-#ifdef _LIBCPP_BIG_ENDIAN
-    static const size_type __short_mask = 0x01;
-    static const size_type __long_mask  = 0x1ul;
-#else  // _LIBCPP_BIG_ENDIAN
-    static const size_type __short_mask = 0x80;
-    static const size_type __long_mask  = ~(size_type(~0) >> 1);
-#endif // _LIBCPP_BIG_ENDIAN
-
     enum {__min_cap = (sizeof(__long) - 1)/sizeof(value_type) > 2 ?
                       (sizeof(__long) - 1)/sizeof(value_type) : 2};
 
     struct __short
     {
         value_type __data_[__min_cap];
         unsigned char __padding[sizeof(value_type) - 1];
-        unsigned char __size_;
+        unsigned char __size_ : 7;
+        unsigned char __is_long_ : 1;
     };
 
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 2;
+#else
+    static const size_type __endian_factor = 1;
+#endif
+
 #else
 
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 1;
+#else
+    static const size_type __endian_factor = 2;
+#endif
+
     struct __long
     {
-        size_type __cap_;
+        bool __is_long_ : 1;
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
         size_type __size_;
         pointer   __data_;
     };
 
-#ifdef _LIBCPP_BIG_ENDIAN
-    static const size_type __short_mask = 0x80;
-    static const size_type __long_mask  = ~(size_type(~0) >> 1);
-#else  // _LIBCPP_BIG_ENDIAN
-    static const size_type __short_mask = 0x01;
-    static const size_type __long_mask  = 0x1ul;
-#endif // _LIBCPP_BIG_ENDIAN
-
     enum {__min_cap = (sizeof(__long) - 1)/sizeof(value_type) > 2 ?
                       (sizeof(__long) - 1)/sizeof(value_type) : 2};
 
     struct __short
     {
         union
         {
-            unsigned char __size_;
+            struct {
+                bool __is_long_ : 1;
+                unsigned char __size_ : 7;
+            };
             value_type __lx;
         };
         value_type __data_[__min_cap];
     };
 
 #endif // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
     union __ulx{__long __lx; __short __lxx;};
#endif
 
     _LIBCPP_INLINE_VISIBILITY bool __invariants() const;
 
     _LIBCPP_INLINE_VISIBILITY void __clear_and_shrink() _NOEXCEPT;
 
     _LIBCPP_INLINE_VISIBILITY void __shrink_or_extend(size_type __target_capacity);
 
     _LIBCPP_INLINE_VISIBILITY
-    bool __is_long() const _NOEXCEPT
-        {return bool(__r_.first().__s.__size_ & __short_mask);}
+    bool __is_long() const _NOEXCEPT {
+        return __r_.first().__s.__is_long_;
+    }
 
 #if _LIBCPP_DEBUG_LEVEL == 2
 
     bool __dereferenceable(const const_iterator* __i) const;
     bool __decrementable(const const_iterator* __i) const;
     bool __addable(const const_iterator* __i, ptrdiff_t __n) const;
     bool __subscriptable(const const_iterator* __i, ptrdiff_t __n) const;
 
iterator __insert_from_safe_copy(size_type __n, size_type __ip, _ForwardIterator __first, _ForwardIterator __last) {
             traits_type::assign(*__p, *__first);
 
         return begin() + __ip;
     }
 
     _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __r_.second(); }
     _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __r_.second(); }
 
-#ifdef _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
-
     _LIBCPP_INLINE_VISIBILITY
-    void __set_short_size(size_type __s) _NOEXCEPT
-#   ifdef _LIBCPP_BIG_ENDIAN
-        {__r_.first().__s.__size_ = (unsigned char)(__s << 1);}
-#   else
-        {__r_.first().__s.__size_ = (unsigned char)(__s);}
-#   endif
-
-    _LIBCPP_INLINE_VISIBILITY
-    size_type __get_short_size() const _NOEXCEPT
-#   ifdef _LIBCPP_BIG_ENDIAN
-        {return __r_.first().__s.__size_ >> 1;}
-#   else
-        {return __r_.first().__s.__size_;}
-#   endif
-
-#else  // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __set_short_size(size_type __s) _NOEXCEPT
-#   ifdef _LIBCPP_BIG_ENDIAN
-        {__r_.first().__s.__size_ = (unsigned char)(__s);}
-#   else
-        {__r_.first().__s.__size_ = (unsigned char)(__s << 1);}
-#   endif
+    void __set_short_size(size_type __s) _NOEXCEPT {
+        if (__s > 127)
+            __libcpp_unreachable();
+        __r_.first().__s.__size_ = __s;
+        __r_.first().__s.__is_long_ = false;
+    }
 
     _LIBCPP_INLINE_VISIBILITY
-    size_type __get_short_size() const _NOEXCEPT
-#   ifdef _LIBCPP_BIG_ENDIAN
-        {return __r_.first().__s.__size_;}
-#   else
-        {return __r_.first().__s.__size_ >> 1;}
-#   endif
-
-#endif // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
+    size_type __get_short_size() const _NOEXCEPT {
+        if (__r_.first().__s.__is_long_)
+            __libcpp_unreachable();
+        return __r_.first().__s.__size_;
+    }
 
     _LIBCPP_INLINE_VISIBILITY
     void __set_long_size(size_type __s) _NOEXCEPT
         {__r_.first().__l.__size_ = __s;}
     _LIBCPP_INLINE_VISIBILITY
     size_type __get_long_size() const _NOEXCEPT
         {return __r_.first().__l.__size_;}
     _LIBCPP_INLINE_VISIBILITY
     void __set_size(size_type __s) _NOEXCEPT
         {if (__is_long()) __set_long_size(__s); else __set_short_size(__s);}
 
     _LIBCPP_INLINE_VISIBILITY
-    void __set_long_cap(size_type __s) _NOEXCEPT
-        {__r_.first().__l.__cap_  = __long_mask | __s;}
+    void __set_long_cap(size_type __s) _NOEXCEPT {
+        __r_.first().__l.__cap_ = __s / __endian_factor;
+        __r_.first().__l.__is_long_ = true;
+    }
+
     _LIBCPP_INLINE_VISIBILITY
-    size_type __get_long_cap() const _NOEXCEPT
-        {return __r_.first().__l.__cap_ & size_type(~__long_mask);}
+    size_type __get_long_cap() const _NOEXCEPT {
+        return __r_.first().__l.__cap_ * __endian_factor;
+    }
 
     _LIBCPP_INLINE_VISIBILITY
     void __set_long_pointer(pointer __p) _NOEXCEPT
         {__r_.first().__l.__data_ = __p;}
     _LIBCPP_INLINE_VISIBILITY
     pointer __get_long_pointer() _NOEXCEPT
         {return __r_.first().__l.__data_;}
     _LIBCPP_INLINE_VISIBILITY

 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 typename basic_string<_CharT, _Traits, _Allocator>::size_type
 basic_string<_CharT, _Traits, _Allocator>::max_size() const _NOEXCEPT
 {
     size_type __m = __alloc_traits::max_size(__alloc());
-#ifdef _LIBCPP_BIG_ENDIAN
-    return (__m <= ~__long_mask ? __m : __m/2) - __alignment;
-#else
-    return __m - __alignment;
-#endif
+    return ((__m <= std::numeric_limits<size_type>::max() / 2) ? __m : __m / __endian_factor) - __alignment;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 void
 basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __requested_capacity)
 {
     if (__requested_capacity > max_size())
         __throw_length_error();
 
     // Make sure reserve(n) never shrinks. This is technically only required in C++20
     // and later (since P0966R1), however we provide consistent behavior in all Standard
     // modes because this function is instantiated in the shared library.