2/2: Fix `TestDataFormatterStdList` regression

[jankratochvil]

Summary

Since D66174 I see failures of TestDataFormatterStdList in about 50% of runs on Fedora 30 x86_64 libstdc++.
I have found out that LLDB internally expects these RegularExpressions to be matched in their alphabetical order:

^std::(__cxx11::)?list<.+>(( )?&)?$
^std::__[[:alnum:]]+::list<.+>(( )?&)?$

But since D66174 they are sometimes matched in reverse order. A simple ugly workaround is to just sort it for ite ration. The overlapping matches one can see by another debug patch on top of it.
In fact it was only some luck it worked before as there is internally std::map<lldb::RegularExpressionSP, FormatterImpl> (FormattersContainer).

[jankratochvil]

From IRC:
In fact the whole patch is a oneliner (+operator< for RegularExpression.h):

-  typedef FormattersContainer<lldb::RegularExpressionSP, FormatterImpl> RegexMatchContainer;
+  typedef FormattersContainer<RegularExpression, FormatterImpl> RegexMatchContainer;

The rest of the patch is just a mechanical update to make it compile again.

It was my mistake when trying to fix the TestDataFormatterStdList regression that it looked to me the new RegularExpression must be broken (its lifetime). In the end it was not broken. But along the way I have removed its IMO-excessive Compile() method as I am trying to push this API change as a cleanup in D66392. If there is disagreement I will rework this patch D66398 to use the former RegularExpression API using Compile(). If there is disagreement even with that I can drop it all and just make a new comparison function for lldb::RegularExpressionSP.

I find the change from std::shared_ptr to rvalues is a nice cleanup+optimization anyway. I do not like much std::shared_ptr myself as due to its lock instructions it kills performance of manycore machines.

[labath]

I very much support getting rid of shared pointers, and for this is I am inclined to accept this. However, I don't think it was ever really intended to use the sorting order of the regex text as a disambiguating criterion for data formatters, and I don't think we should make a feature of it now -- it's just too weird and I doubt anybody will expect that.

Since it seems that there is a need for disambiguation (libc++ can be configured to have any name for the inline namespace, so there's no way to make its regex not match the libstdc++ one), I think we should just use the registration order to disambiguate. Then we just need to make sure the libstdc++ formatter is registered before the libc++ one, which should be both easy to achieve and understandable.

Since you've recently touched this code, how hard do you think it would be to exchange the std::map in FormattersContainer for a std::vector? Would you be willing to give that a try?

[JDevlieghere]

lldb/source/DataFormatters/FormatManager.cpp

950 ↗

(On Diff #215895)

I'd just remove this

[jankratochvil]

In D66398#1635344, @labath wrote:

Since it seems that there is a need for disambiguation (libc++ can be configured to have any name for the inline namespace, so there's no way to make its regex not match the libstdc++ one),

This is regression from D57466. If you configure libc++ to have the same name ("__cxx11") as libstdc++ then LLDB really cannot recognize it is libc++ behind.

I think we should just use the registration order to disambiguate. Then we just need to make sure the libstdc++ formatter is registered before the libc++ one, which should be both easy to achieve and understandable.

Since you've recently touched this code, how hard do you think it would be to exchange the std::map in FormattersContainer for a std::vector? Would you be willing to give that a try?

From IRC:
That std::vector there is not easy. First some methods were already using .find() on it so I had to change them to iterate through that vector.
Also some iterating in FormattersContainer (Delete_Impl, GetExact_Impl) were now iterating for comparison despite they could use .find() so by moving to std::vector we lose this improvement possibility.
labath: if replace that by a single entry then the nondeterminism goes away
labath: std::.*::list -> { if (inline_namespace == "__cxx11") return "libstdc++ formatter" else return "libc++ formatter" }

I have somehow find that dispatching implemented C++ not so simple, why not just to remove the ambiguity from the regexes?

[jankratochvil]

It just makes now the regexes unambiguous.

[labath]

In D66398#1640840, @jankratochvil wrote:

It just makes now the regexes unambiguous.

I think this is fine too, but I would also only consider it a stop-gap, because these kinds of regular are horrid.

What was the problem with the dispatching solution? If it is hard to programatically access the inline namespace name (though I expect you should at least be able to to something like type.GetName().startswith("std::__cxx11"), then we can dispatch based on some other property. The presence of some member variable for instance? This would technically be even more correct as it would also work in the case when someone sadistically configures libc++ to use the __cxx11 namespace. And since the formatters need have knowledge of the member variables then I think dispatching based on them is fair game too...

lldb/include/lldb/DataFormatters/FormattersContainer.h

299–316 ↗

(On Diff #216585)

Let's make a separate patch for this. I think it would be better to print this error through proper channels, but that might require some plumbing.

[jankratochvil]

In D66398#1640871, @labath wrote:

because these kinds of regular are horrid.

Yes, they are. So let's link with PCRE (BSD licensed).

What was the problem with the dispatching solution?

I do not find it so easy as libc++ formatter is using static function LibcxxStdListSyntheticFrontEndCreator while libstdc++ is using new object instance ScriptedSyntheticChildren. The dispatching should be in LibcxxStdListSyntheticFrontEndCreator so should it store new ScriptedSyntheticChildren as a static variable?

And then I do not like that solution anyway. Currently the dispatching is based on type name strings so why not to just do that? KISS

we can dispatch based on some other property. The presence of some member variable for instance?

So let's drop the type name completely and make the matching duck typed. Why to divert from how it is currently done?

Anyway sure feel free to rewrite it but the regression is fixed now.

[jankratochvil]

"jankratochvil added a reverting change" - that is not true, Differential got somehow confused by the commit text there talking about this patch.

[labath]

In D66398#1677414, @jankratochvil wrote:

"jankratochvil added a reverting change" - that is not true, Differential got somehow confused by the commit text there talking about this patch.

Good to know, but assuming that the above patch sticks, the plan *is* to revert this, right?

[jankratochvil]

In D66398#1679071, @labath wrote:

Good to know, but assuming that the above patch sticks, the plan *is* to revert this, right?

D66654 is already checked-in, I have considered it as done as is. If you want to revert this patch then:

• One needs to reverse the registration order as a simple revert fails for TestDataFormatterStdList.py
• One should put a big fat warning there.

Should I do that?

lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

AddCXXSynthetic(
       lldb_private::formatters::LibcxxStdForwardListSyntheticFrontEndCreator,
       "libc++ std::forward_list synthetic children",
       ConstString("^std::__[[:alnum:]]+::forward_list<.+>(( )?&)?$"),
       stl_synth_flags, true);
   AddCXXSynthetic(
       cpp_category_sp,
       lldb_private::formatters::LibcxxStdListSyntheticFrontEndCreator,
       "libc++ std::list synthetic children",
-      ConstString("^std::__[[:alnum:]]+::list<.+>(( )?&)?$"), stl_deref_flags,
-      true);
+      // A POSIX variant of: "^std::__(?!cxx11:)[[:alnum:]]+::list<.+>(( )?&)?$"
+      // so that it does not clash with: "^std::(__cxx11::)?list<.+>(( )?&)?$"
+      ConstString("^std::__([A-Zabd-z0-9]|cx?[A-Za-wyz0-9]|cxx1?[A-Za-z02-9]|"
+                  "cxx11[[:alnum:]])[[:alnum:]]*::list<.+>(( )?&)?$"),
+      stl_deref_flags, true);
   AddCXXSynthetic(
       cpp_category_sp,
       lldb_private::formatters::LibcxxStdMapSyntheticFrontEndCreator,
       "libc++ std::map synthetic children",
       ConstString("^std::__[[:alnum:]]+::map<.+> >(( )?&)?$"), stl_synth_flags,
       true);
   AddCXXSynthetic(
       cpp_category_sp,
AddCXXSummary(cpp_category_sp,
                 "libc++ std::vector summary provider",
                 ConstString("^std::__[[:alnum:]]+::vector<.+>(( )?&)?$"),
                 stl_summary_flags, true);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::list summary provider",
                 ConstString("^std::__[[:alnum:]]+::forward_list<.+>(( )?&)?$"),
                 stl_summary_flags, true);
-  AddCXXSummary(cpp_category_sp,
-                lldb_private::formatters::LibcxxContainerSummaryProvider,
-                "libc++ std::list summary provider",
-                ConstString("^std::__[[:alnum:]]+::list<.+>(( )?&)?$"),
-                stl_summary_flags, true);
+  AddCXXSummary(
+      cpp_category_sp, lldb_private::formatters::LibcxxContainerSummaryProvider,
+      // A POSIX variant of: "^std::__(?!cxx11:)[[:alnum:]]+::list<.+>(( )?&)?$"
+      // so that it does not clash with: "^std::(__cxx11::)?list<.+>(( )?&)?$"
+      ConstString("^std::__([A-Zabd-z0-9]|cx?[A-Za-wyz0-9]|cxx1?[A-Za-z02-9]|"
+                  "cxx11[[:alnum:]])[[:alnum:]]*::list<.+>(( )?&)?$"),
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::map summary provider",
                 ConstString("^std::__[[:alnum:]]+::map<.+>(( )?&)?$"),
                 stl_summary_flags, true);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::deque summary provider",