[libcxx] avoid using anonymous struct with base classes (fixes gcc-12)

[azat]

Summary

g++-12 reports:

libcxx/include/string:727:13: error: anonymous struct with base classes
  727 |             : __padding<value_type>

[ldionne]

libcxx/include/string

707

This is an ABI break, since __padding<char, 1> is normally elided via the empty base optimization. This change prevents that from happening.

Instead, I believe we should give a name to this anonymous struct, since the lack thereof is non-standard and also pretty surprising. I would do:

struct __short
{
    value_type __data_[__min_cap];
    struct
        : __padding<value_type>
    {
        unsigned char __size_;
    } __padded_;
};

Then, you'll have to change places where we access .__size_ directly into accessing .__padded_.__size_ instead. Note that __padded_ isn't a great name, but I couldn't think of anything better. Suggestions are welcome.

[philnik]

Why not make it something like

template <class CharT>
struct short_size {
    char __padding[sizeof(CharT) - 1];
    unsigned char __size_;
};

? That would eliminate the need for __padded all together. This of course requires another extension, but makes the code a lot simpler.

[azat]

v2: avoid ABI breakage (thanks to tests and @ldionne)

[azat]

@ldionne @philnik can you please take a look? Thanks in advance.

[philnik]

This LGTM, but @ldionne has to says if he is OK with using the extension.

[ldionne]

So I was wondering how to make sure this change is good, and wasn't too sure. It turns out that we don't have any ABI compatibility testing for this code path. I'm adding that in D123081, so I'd like for D123081 to land before we land this. Once we have backdeployment testing running on arm64, we can try messing up the padding here in a subtle way, confirm that it breaks our tests, and then move forward with this patch with confidence (and satisfaction knowing our tests really have our back).

This looks correct to me, but still requesting changes so it shows up in the review queue.

libcxx/include/string

709

This should be unsigned char __padding[...]. It doesn't change anything, but it makes it clearer that we're padding the unsigned char __size_ member that follows.

[azat]

libcxx/include/string

709

So I should made this change once https://reviews.llvm.org/D123081 will land, right?

[ldionne]

libcxx/include/string

709

You can make this change right now, it's just that I'd like to confirm that we'd catch any issue with this patch before actually landing it in the tree!

[azat]

Use unsigned char for __padding (as noted by @ldionne)

[azat]

ldionne should I resubmit the patch to run CI against https://reviews.llvm.org/D123081?

As for failures, they looks unrelated?

• MacOS arm64 - Assertion failed: (seq++ == n_), function ~OrderChecker, file thread_local_destruction_order.pass.cpp, line 24.
• Apple back-deployment macosx11.0 arm64 - Assertion failed: (globalMemCounter.checkOutstandingNewEq(1)), function main, file move.pass.cpp, line 33.

[ldionne]

@azat Please rebase on top of main and then re-upload the patch. It should run on top of the fixed CI and I would expect that everything looks green!

[azat]

Rebase on top of main

[ldionne]

And I just applied your original patch locally on my arm64 macbook air, ran the macOS 11 backdeployment testing, and I can see numerous tests failing because strings have an incorrect length. So this issue would have been caught with macOS arm64 backdeployment CI.

With this additional confidence, this LGTM if CI is passing. Please provide Author Name <email@domain> if you need us to commit this for you. Thanks!

[azat]

And I just applied your original patch locally on my arm64 macbook air, ran the macOS 11 backdeployment testing, and I can see numerous tests failing because strings have an incorrect length. So this issue would have been caught with macOS arm64 backdeployment CI.

Great, thanks!

Please provide Author Name <email@domain> if you need us to commit this for you.

Azat Khuzhin <[e-mail]>

Interesting, I though that should be included in the patch on phabricator.

[jgorbe]

Is it possible that this change just broke the lldb data formatter for string?

[azat]

Is it possible that this change just broke the lldb data formatter for string?

@jgorbe AFAICS - No, have you experience some issues?

Plus I see there are tests for string formatting in lldb, which cover both: long and short strings:

[jgorbe]

Thanks for the reply! Yes, I've seen some test failures while trying to update our llvm to (closer to) HEAD, and I bisected them to this commit. These were the affected tests:

lldb/test/API/commands/expression/call-function/TestCallStdStringFunction.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-skip-summary/TestDataFormatterSkipSummary.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/list/TestDataFormatterGenericList.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multiset/TestDataFormatterGenericMultiSet.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py.test
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py.test
lldb/test/API/functionalities/data-formatter/stringprinter/TestStringPrinter.py.test
lldb/test/API/python_api/sbvalue_persist/TestSBValuePersist.py.test

I believe all of them failed with some string output being replaced by "Summary Unavailable". My first attempt at reproducing it in a plain upstream checkout failed (ninja check-lldb showed me lots of errors, both before and after this change, but it's very possible that I was doing something wrong with my local build), so I added the comment here to see if anyone else was experiencing the same problem, or if the problem was in our side. If you're not seeing the problem, that's additional evidence that the problem might be on our side.

libcxx/include/string

#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];
         struct
-            : __padding<value_type>
         {
+            __padding<value_type> __padding_;
             unsigned char __size_;
         };
     };
 
 #else
 
     struct __long
     {
         size_type __cap_;
         size_type __size_;