[Lldb-commits] [PATCH] D66392: Fix `TestDataFormatterStdList` regression + D66174 follow-up/cleanup

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: JDevlieghere, labath, jingham, clayborg.
jankratochvil added a project: LLDB.
Herald added a reviewer: jdoerfert.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Since D66174 <https://reviews.llvm.org/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 `RegularExpression`s to be matched in their alphabetical order:

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

But since D66174 <https://reviews.llvm.org/D66174> they are sometimes matched in reverse order. In fact it was only some luck it worked before as there is internally `std::map<lldb::RegularExpressionSP, FormatterImpl>` (`FormattersContainer`).
So I have changed the key `RegularExpressionSP` -> `RegularExpression` and moved it everywhere as rvalue where possible (so that I am not accused of a performance regression; except for 2 cases needing real copy-ctor).

Then there is a second problem that the new `RegularExpression` from D66174 <https://reviews.llvm.org/D66174> IMO contains stale string references:

  bool RegularExpression::Compile(llvm::StringRef str) {
    m_regex_text = str;
    m_regex = llvm::Regex(str);

Caller will pass a string reference which `RegularExpression` stores locally but its `Regex` still points to the caller's string. Maybe currently the caller really keeps the string lifetime valid (although what is for example `lldb/source/Commands/CommandCompletions.cpp` the line 451 stale reference?) but I find it still fragile. If the caller's string lifetime is really guaranteed to be remain valid then why isn't `m_regex_text` a `StringRef`? If we rather change the `Regex` to reference `m_regex_text` instead then it will reference stale memory after executing its default move-ctor:

  RegularExpression(RegularExpression &&rhs) = default;

In the patch below I copy the string to local `m_regex_text` and initializing `Regex` only lazily and dropping/forgetting it during move&copy&assignment ctors.
BTW there was forgotten unused inheritance: `class RegularExpression : public llvm::Regex`
I have changed (IMO simplified) the `RegularExpression` API a bit for easier laziness of its `Regex`. Callers were using the `Compile` method without checking its return code which would defeat the laziness without any purpose.
The patch is big addressing two different issues (`FormattersContainer` ordering and `RegularExpression` stale string references) but I find it double the work just to separate it into 2 patches.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66392

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverName.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  lldb/include/lldb/DataFormatters/TypeCategory.h
  lldb/include/lldb/Interpreter/OptionValueRegex.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Utility/RegularExpression.h
  lldb/source/API/SBTarget.cpp
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Core/AddressResolverName.cpp
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/FormattersHelpers.cpp
  lldb/source/Interpreter/CommandObjectRegexCommand.cpp
  lldb/source/Interpreter/OptionValueRegex.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Utility/RegularExpression.cpp
  llvm/include/llvm/Support/Regex.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66392.215776.patch
Type: text/x-patch
Size: 45055 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190818/d248e1e6/attachment-0001.bin>