Windows :) But that was before std::regex. In theory std::regex would work everywhere (although idk how it performs) On Wed, Sep 21, 2016 at 6:04 PM Jim Ingham <jing...@apple.com> wrote:
> It seems a little odd that llvm has its own forked copy of Henry Spencer's > old regular expression engine? Are there platforms we care about that > don't have a maintained version of exactly the same code? > > Jim > > > > On Sep 21, 2016, at 5:42 PM, Zachary Turner <ztur...@google.com> wrote: > > > > I'll try to address the Regex issue this week (by making everything use > llvm extended mode regexes). If someone feels up to the challenge, adding > support for \d \s etc etc to llvm's regex implementation would make a lot > of people very happy. > > > > On Wed, Sep 21, 2016 at 5:38 PM Jim Ingham <jing...@apple.com> wrote: > > > > > On Sep 21, 2016, at 5:25 PM, Greg Clayton via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > > > > > Yep, and we can't have any regex objects in LLDB using those because > they will only work on Apple and we don't want code like: > > > > > > #if defined(__APPLE__) > > > RegularExpression e("\s+"); > > > #else > > > RegularExpression e("[ \t]+"); > > > #endif > > > > > > I know "\s" is probably extended, so this was a bad example, but you > get my drift. > > > > Nope, sadly for extended you have to say [[:space:]]. To avoid the > #define problem, we could require only extended regular expressions in lldb > code, but let users type the more convenient enhanced one wherever the > command line uses them. But beyond these convenient shortcuts there > probably aren't that many additions that would be useful for the kind of > regular expressions our users are likely to write. If we ever provide "-r" > option to "memory find" the byte literal extension might come in handy. > But I don't think that justifies making MacOS builds different. > > > > If it really bothered us we could go get a more modern regex engine from > somewhere (rip it out of Tcl or something like that...) > > > > Jim > > > > > > > > > > Greg > > > > > >> On Sep 21, 2016, at 5:19 PM, Zachary Turner <ztur...@google.com> > wrote: > > >> > > >> That sounds like a plan. BTW, extended is the one that everyone > supports, enhanced is the one that only apple supports. > > >> > > >> On Wed, Sep 21, 2016 at 5:18 PM Greg Clayton <gclay...@apple.com> > wrote: > > >> And we should check for any "extended" mode regex stuff and get rid > of it since as you said they are not portable. They tend to be shortcuts > for classes of objects and we can just fix the regex to be more explicit > about it. For now we would keep the lldb_private::RegularExpression class, > have it have a llvm::Regex member and then lldbassert if the regex fails to > compile so we can catch any extended cruft that we might miss so we can fix > it... > > >> > > >>> On Sep 21, 2016, at 5:15 PM, Greg Clayton <gclay...@apple.com> > wrote: > > >>> > > >>> To be clear: if we can make StringRef work efficiently, I am fine > with that. We can just cut over to using llvm::Regex where it uses the > start and end pointer. I would just like to avoid making string copies for > no reason. I don't have anything against using StringRef as long as we do > it efficiently. > > >>> > > >>> Greg > > >>> > > >>> > > >>>> On Sep 21, 2016, at 5:13 PM, Greg Clayton via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > >>>> > > >>>>> > > >>>>> On Sep 21, 2016, at 4:43 PM, Zachary Turner <ztur...@google.com> > wrote: > > >>>>> > > >>>>> You need to duplicate something on the heap once when you execute > the regex. And in turn you save tens or hundreds or copies on the way > there because of inefficient string usage. > > >>>> > > >>>> Where? please show this. > > >>>> > > >>>> I see the following callers: > > >>>> > > >>>> const char *class_name = > > >>>> iterator->second->GetClassName().AsCString("<unknown>"); > > >>>> if (regex_up && class_name && > > >>>> !regex_up->Execute(llvm::StringRef(class_name))) > > >>>> > > >>>> You are adding a strlen() call here to construct the StringRef, not > saving anything. > > >>>> > > >>>> > > >>>> bool CommandObjectRegexCommand::DoExecute(const char *command, > > >>>> CommandReturnObject &result) > { > > >>>> if (command) { > > >>>> EntryCollection::const_iterator pos, end = m_entries.end(); > > >>>> for (pos = m_entries.begin(); pos != end; ++pos) { > > >>>> RegularExpression::Match regex_match(m_max_matches); > > >>>> > > >>>> if (pos->regex.Execute(command, ®ex_match)) { > > >>>> std::string new_command(pos->command); > > >>>> std::string match_str; > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor. > > >>>> > > >>>> > > >>>> DataVisualization::Categories::ForEach( > > >>>> [®ex, &result](const lldb::TypeCategoryImplSP &category_sp) > -> bool { > > >>>> if (regex) { > > >>>> bool escape = true; > > >>>> if (regex->GetText() == category_sp->GetName()) { > > >>>> escape = false; > > >>>> } else if (regex->Execute(llvm::StringRef::withNullAsEmpty( > > >>>> category_sp->GetName()))) { > > >>>> escape = false; > > >>>> } > > >>>> > > >>>> if (escape) > > >>>> return true; > > >>>> } > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor. > > >>>> > > >>>> > > >>>> TypeCategoryImpl::ForEachCallbacks<FormatterType> foreach; > > >>>> foreach > > >>>> .SetExact([&result, &formatter_regex, &any_printed]( > > >>>> ConstString name, > > >>>> const FormatterSharedPointer &format_sp) -> bool > { > > >>>> if (formatter_regex) { > > >>>> bool escape = true; > > >>>> if (name.GetStringRef() == formatter_regex->GetText()) { > > >>>> escape = false; > > >>>> } else if (formatter_regex->Execute(name.GetStringRef())) { > > >>>> escape = false; > > >>>> } > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor. > > >>>> > > >>>> bool ParseCoordinate(const char *id_cstr) { > > >>>> RegularExpression regex; > > >>>> RegularExpression::Match regex_match(3); > > >>>> > > >>>> llvm::StringRef id_ref = > llvm::StringRef::withNullAsEmpty(id_cstr); > > >>>> bool matched = false; > > >>>> if > (regex.Compile(llvm::StringRef("^([0-9]+),([0-9]+),([0-9]+)$")) && > > >>>> regex.Execute(id_ref, ®ex_match)) > > >>>> matched = true; > > >>>> else if (regex.Compile(llvm::StringRef("^([0-9]+),([0-9]+)$")) && > > >>>> regex.Execute(id_ref, ®ex_match)) > > >>>> matched = true; > > >>>> else if (regex.Compile(llvm::StringRef("^([0-9]+)$")) && > > >>>> regex.Execute(id_ref, ®ex_match)) > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor. > > >>>> > > >>>> void DWARFCompileUnit::ParseProducerInfo() { > > >>>> m_producer_version_major = UINT32_MAX; > > >>>> m_producer_version_minor = UINT32_MAX; > > >>>> m_producer_version_update = UINT32_MAX; > > >>>> > > >>>> const DWARFDebugInfoEntry *die = GetCompileUnitDIEPtrOnly(); > > >>>> if (die) { > > >>>> > > >>>> const char *producer_cstr = die->GetAttributeValueAsString( > > >>>> m_dwarf2Data, this, DW_AT_producer, NULL); > > >>>> if (producer_cstr) { > > >>>> RegularExpression llvm_gcc_regex( > > >>>> llvm::StringRef("^4\\.[012]\\.[01] \\(Based on Apple " > > >>>> "Inc\\. build [0-9]+\\) \\(LLVM build " > > >>>> "[\\.0-9]+\\)$")); > > >>>> if (llvm_gcc_regex.Execute(llvm::StringRef(producer_cstr))) { > > >>>> m_producer = eProducerLLVMGCC; > > >>>> } else if (strstr(producer_cstr, "clang")) { > > >>>> static RegularExpression g_clang_version_regex( > > >>>> llvm::StringRef("clang-([0-9]+)\\.([0-9]+)\\.([0-9]+)")); > > >>>> RegularExpression::Match regex_match(3); > > >>>> if > (g_clang_version_regex.Execute(llvm::StringRef(producer_cstr), > > >>>> ®ex_match)) { > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor (2 of them mind you). > > >>>> > > >>>> void DWARFDebugPubnamesSet::Find( > > >>>> const RegularExpression ®ex, > > >>>> std::vector<dw_offset_t> &die_offset_coll) const { > > >>>> DescriptorConstIter pos; > > >>>> DescriptorConstIter end = m_descriptors.end(); > > >>>> for (pos = m_descriptors.begin(); pos != end; ++pos) { > > >>>> if (regex.Execute(pos->name.c_str())) > > >>>> die_offset_coll.push_back(m_header.die_offset + pos->offset); > > >>>> } > > >>>> } > > >>>> > > >>>> No copy saved. Just wasted time with strlen in StringRef > constructor (2 of them mind you). > > >>>> > > >>>> > > >>>> std::string slice_str; > > >>>> if (reg_info_dict->GetValueForKeyAsString("slice", slice_str, > nullptr)) { > > >>>> // Slices use the following format: > > >>>> // REGNAME[MSBIT:LSBIT] > > >>>> // REGNAME - name of the register to grab a slice of > > >>>> // MSBIT - the most significant bit at which the current > register value > > >>>> // starts at > > >>>> // LSBIT - the least significant bit at which the current > register value > > >>>> // ends at > > >>>> static RegularExpression g_bitfield_regex( > > >>>> > llvm::StringRef("([A-Za-z_][A-Za-z0-9_]*)\\[([0-9]+):([0-9]+)\\]")); > > >>>> RegularExpression::Match regex_match(3); > > >>>> if (g_bitfield_regex.Execute(slice_str, ®ex_match)) { > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> void SourceManager::File::FindLinesMatchingRegex( > > >>>> RegularExpression ®ex, uint32_t start_line, uint32_t end_line, > > >>>> std::vector<uint32_t> &match_lines) { > > >>>> match_lines.clear(); > > >>>> > > >>>> if (!LineIsValid(start_line) || > > >>>> (end_line != UINT32_MAX && !LineIsValid(end_line))) > > >>>> return; > > >>>> if (start_line > end_line) > > >>>> return; > > >>>> > > >>>> for (uint32_t line_no = start_line; line_no < end_line; line_no++) { > > >>>> std::string buffer; > > >>>> if (!GetLine(line_no, buffer)) > > >>>> break; > > >>>> if (regex.Execute(buffer)) { > > >>>> match_lines.push_back(line_no); > > >>>> } > > >>>> } > > >>>> } > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> > > >>>> static dw_offset_t > > >>>> FindCallbackString(SymbolFileDWARF *dwarf2Data, DWARFCompileUnit > *cu, > > >>>> DWARFDebugInfoEntry *die, const dw_offset_t > next_offset, > > >>>> const uint32_t curr_depth, void *userData) { > > >>>> FindCallbackStringInfo *info = (FindCallbackStringInfo *)userData; > > >>>> > > >>>> if (!die) > > >>>> return next_offset; > > >>>> > > >>>> const char *die_name = die->GetName(dwarf2Data, cu); > > >>>> if (!die_name) > > >>>> return next_offset; > > >>>> > > >>>> if (info->regex) { > > >>>> if (info->regex->Execute(llvm::StringRef(die_name))) > > >>>> info->die_offsets.push_back(die->GetOffset()); > > >>>> } else { > > >>>> if ((info->ignore_case ? strcasecmp(die_name, info->name) > > >>>> : strcmp(die_name, info->name)) == 0) > > >>>> info->die_offsets.push_back(die->GetOffset()); > > >>>> } > > >>>> > > >>>> // Just return the current offset to parse the next CU or DIE entry > > >>>> return next_offset; > > >>>> } > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> > > >>>> bool Get_Impl(ConstString key, MapValueType &value, > > >>>> lldb::RegularExpressionSP *dummy) { > > >>>> llvm::StringRef key_str = key.GetStringRef(); > > >>>> std::lock_guard<std::recursive_mutex> guard(m_format_map.mutex()); > > >>>> MapIterator pos, end = m_format_map.map().end(); > > >>>> for (pos = m_format_map.map().begin(); pos != end; pos++) { > > >>>> lldb::RegularExpressionSP regex = pos->first; > > >>>> if (regex->Execute(key_str)) { > > >>>> value = pos->second; > > >>>> return true; > > >>>> } > > >>>> } > > >>>> return false; > > >>>> } > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> > > >>>> PacketResult echo_packet_result = > > >>>> SendPacketNoLock(llvm::StringRef(echo_packet, > echo_packet_len)); > > >>>> if (echo_packet_result == PacketResult::Success) { > > >>>> const uint32_t max_retries = 3; > > >>>> uint32_t successful_responses = 0; > > >>>> for (uint32_t i = 0; i < max_retries; ++i) { > > >>>> StringExtractorGDBRemote echo_response; > > >>>> echo_packet_result = > WaitForPacketWithTimeoutMicroSecondsNoLock( > > >>>> echo_response, timeout_usec, false); > > >>>> if (echo_packet_result == PacketResult::Success) { > > >>>> ++successful_responses; > > >>>> if > (response_regex.Execute(echo_response.GetStringRef())) { > > >>>> sync_success = true; > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> > > >>>> OptionValueSP Instruction::ReadArray(FILE *in_file, Stream > *out_stream, > > >>>> OptionValue::Type data_type) { > > >>>> bool done = false; > > >>>> char buffer[1024]; > > >>>> > > >>>> OptionValueSP option_value_sp(new OptionValueArray(1u << > data_type)); > > >>>> > > >>>> int idx = 0; > > >>>> while (!done) { > > >>>> if (!fgets(buffer, 1023, in_file)) { > > >>>> out_stream->Printf( > > >>>> "Instruction::ReadArray: Error reading file (fgets).\n"); > > >>>> option_value_sp.reset(); > > >>>> return option_value_sp; > > >>>> } > > >>>> > > >>>> std::string line(buffer); > > >>>> > > >>>> size_t len = line.size(); > > >>>> if (line[len - 1] == '\n') { > > >>>> line[len - 1] = '\0'; > > >>>> line.resize(len - 1); > > >>>> } > > >>>> > > >>>> if ((line.size() == 1) && line[0] == ']') { > > >>>> done = true; > > >>>> line.clear(); > > >>>> } > > >>>> > > >>>> if (!line.empty()) { > > >>>> std::string value; > > >>>> static RegularExpression g_reg_exp( > > >>>> llvm::StringRef("^[ \t]*([^ \t]+)[ \t]*$")); > > >>>> RegularExpression::Match regex_match(1); > > >>>> bool reg_exp_success = g_reg_exp.Execute(line, ®ex_match); > > >>>> if (reg_exp_success) > > >>>> regex_match.GetMatchAtIndex(line.c_str(), 1, value); > > >>>> else > > >>>> value = line; > > >>>> > > >>>> No copy saved. > > >>>> > > >>>> I could do on with all 31 call sites, but I will stop. All that was > added was inefficiency and requiring us to make a copy of the string before > it is used to evaluate. Some of these are evaluated in tight loops, like > the searching for type summaries and formats that are regex based. Happens > once for each item in a SBValue (and once per child that is displayed... > > >>>> > > >>>>> We could also just un-delete the overload that takes a const > char*, then the duplication would only ever happen when you explicitly use > a StringRef. > > >>>> > > >>>> For execute it is fine to make one that takes a StringRef, but it > would just call .str().c_str() and call the other one. We would assume the > "const char *" version of execute would be null terminated. > > >>>> > > >>>>> > > >>>>> I don't agree this should be reverted. In the process of doing > this conversion I eliminated numerous careless string copies. > > >>>> > > >>>> As I showed above the opposite was true. We made many calls to > strlen to construct the StringRef so we actually made things slower. > > >>>> > > >>>> Greg > > >>>> > > >>>>> > > >>>>> On Wed, Sep 21, 2016 at 4:38 PM Greg Clayton <gclay...@apple.com> > wrote: > > >>>>> This it the perfect example of why not to use a StringRef since > the string needs to be null terminated. Why did we change this? Now even if > you call this function: > > >>>>> > > >>>>> RegularExpression r(...); > > >>>>> > > >>>>> r.Execute(".......................", ...) > > >>>>> > > >>>>> You will need to duplicate the string on the heap just to execute > this. Please revert this. Anything that requires null terminate is not a > candidate for converting to StringRef. > > >>>>> > > >>>>> > > >>>>>> On Sep 21, 2016, at 10:13 AM, Zachary Turner via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > >>>>>> > > >>>>>> Author: zturner > > >>>>>> Date: Wed Sep 21 12:13:51 2016 > > >>>>>> New Revision: 282090 > > >>>>>> > > >>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=282090&view=rev > > >>>>>> Log: > > >>>>>> Fix failing regex tests. > > >>>>>> > > >>>>>> r282079 converted the regular expression interface to accept > > >>>>>> and return StringRefs instead of char pointers. In one case > > >>>>>> a null pointer check was converted to an empty string check, > > >>>>>> but this was an incorrect conversion because an empty string > > >>>>>> is a valid regular expression. Removing this check should > > >>>>>> fix the test failures. > > >>>>>> > > >>>>>> Modified: > > >>>>>> lldb/trunk/source/Core/RegularExpression.cpp > > >>>>>> > > >>>>>> Modified: lldb/trunk/source/Core/RegularExpression.cpp > > >>>>>> URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/RegularExpression.cpp?rev=282090&r1=282089&r2=282090&view=diff > > >>>>>> > ============================================================================== > > >>>>>> --- lldb/trunk/source/Core/RegularExpression.cpp (original) > > >>>>>> +++ lldb/trunk/source/Core/RegularExpression.cpp Wed Sep 21 > 12:13:51 2016 > > >>>>>> @@ -102,7 +102,7 @@ bool RegularExpression::Compile(llvm::St > > >>>>>> > //--------------------------------------------------------------------- > > >>>>>> bool RegularExpression::Execute(llvm::StringRef str, Match > *match) const { > > >>>>>> int err = 1; > > >>>>>> - if (!str.empty() && m_comp_err == 0) { > > >>>>>> + if (m_comp_err == 0) { > > >>>>>> // Argument to regexec must be null-terminated. > > >>>>>> std::string reg_str = str; > > >>>>>> if (match) { > > >>>>>> > > >>>>>> > > >>>>>> _______________________________________________ > > >>>>>> lldb-commits mailing list > > >>>>>> lldb-commits@lists.llvm.org > > >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > >>>>> > > >>>> > > >>>> _______________________________________________ > > >>>> lldb-commits mailing list > > >>>> lldb-commits@lists.llvm.org > > >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > >>> > > >> > > > > > > _______________________________________________ > > > lldb-commits mailing list > > > lldb-commits@lists.llvm.org > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits