[clang-tools-extra] r318840 - [FindAllSymbols] Cache regexes, creating them is expensive

Dimitry Andric via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 25 09:20:42 PST 2017 

Yeah, in the past libc++ broke its own ABI with http://llvm.org/viewvc/llvm-project?view=revision&revision=194536, which is why we had to add the _LIBCPP_TRIVIAL_PAIR_COPY_CTOR hack to our own libc++ __config, in https://svnweb.freebsd.org/base?view=revision&revision=261801.

(Afterwards, in http://llvm.org/viewvc/llvm-project?view=revision&revision=275749, that define was renamed to _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR, but its functionality stayed the same.)

More recently, Eric Fiselier made the hack unnecessary with http://llvm.org/viewvc/llvm-project?view=revision&revision=283944, which I merged into FreeBSD 11-STABLE in https://svnweb.freebsd.org/base?view=revision&revision=315702.

After this, hacks for std::pair having trivial constructors should no longer be nessary, but the fix only made it into FreeBSD 11.1-RELEASE, so users with 11.0-RELEASE are still SOL.

-Dimitry

> On 24 Nov 2017, at 00:01, Krzysztof Parzyszek <kparzysz at codeaurora.org> wrote:
> 
> +Dimitry.
> 
> 
> On 11/23/2017 12:50 PM, Benjamin Kramer wrote:
>> I'm afraid I can't really help you here. You can try twiddling the
>> code a bit by creating the pair explicitly and moving it into the
>> vector, just to avoid the brokenness in the standard library. Not sure
>> if that will help though.
>> On Thu, Nov 23, 2017 at 7:05 PM, Krzysztof Parzyszek
>> <kparzysz at codeaurora.org> wrote:
>>> There has been some problem with std::pair on FreeBSD (due to some ABI
>>> compatibility issue), but I don't know much about what it was.
>>> 
>>> Commenting the reserve doesn't help, unfortunately. The same problem is now
>>> flagged in emplace_back.
>>> 
>>> -Krzysztof
>>> 
>>> 
>>> 
>>> On 11/23/2017 11:47 AM, Benjamin Kramer wrote:
>>>> 
>>>> That looks like a bug in the standard library. Does removing the call
>>>> to reserve fix it? It's not really necessary, that code isn't
>>>> performance sensitive at all.
>>>> 
>>>> On Thu, Nov 23, 2017 at 6:36 PM, Krzysztof Parzyszek
>>>> <kparzysz at codeaurora.org> wrote:
>>>>> 
>>>>> Hi,
>>>>> This broke build on FreeBSD 11:
>>>>> 
>>>>> [100%] Building CXX object
>>>>> 
>>>>> tools/clang/tools/extra/include-fixer/find-all-symbols/CMakeFiles/findAllSymbols.dir/HeaderMapCollector.cpp.o
>>>>> cd /w/bld/org/tools/clang/tools/extra/include-fixer/find-all-symbols &&
>>>>> /w/c/clang+llvm-5.0.0-x86_64-unknown-freebsd11/bin/clang++
>>>>> -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
>>>>> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>>>> -I/w/bld/org/tools/clang/tools/extra/include-fixer/find-all-symbols
>>>>> -I/w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols
>>>>> -I/w/src/llvm.org/tools/clang/include -I/w/bld/org/tools/clang/include
>>>>> -I/w/bld/org/include -I/w/src/llvm.org/include -isystem
>>>>> /usr/local/include
>>>>> -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time
>>>>> -Werror=unguarded-availability-new -std=c++11 -Wall -W
>>>>> -Wno-unused-parameter
>>>>> -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic
>>>>> -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor
>>>>> -Wdelete-non-virtual-dtor -Wno-comment -Wstring-conversion
>>>>> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
>>>>> -Wno-nested-anon-types -O3    -UNDEBUG -fno-exceptions -fno-rtti -o
>>>>> CMakeFiles/findAllSymbols.dir/HeaderMapCollector.cpp.o -c
>>>>> 
>>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>> In file included from
>>>>> 
>>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp:10:
>>>>> In file included from
>>>>> 
>>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.h:13:
>>>>> In file included from /w/src/llvm.org/include/llvm/ADT/StringMap.h:17:
>>>>> In file included from /w/src/llvm.org/include/llvm/ADT/StringRef.h:13:
>>>>> In file included from /w/src/llvm.org/include/llvm/ADT/STLExtras.h:20:
>>>>> In file included from /w/src/llvm.org/include/llvm/ADT/Optional.h:22:
>>>>> In file included from
>>>>> /w/src/llvm.org/include/llvm/Support/type_traits.h:19:
>>>>> /usr/include/c++/v1/utility:315:11: error: call to deleted constructor of
>>>>> 'llvm::Regex'
>>>>>          : first(__p.first),
>>>>>            ^     ~~~~~~~~~
>>>>> /usr/include/c++/v1/memory:1747:31: note: in instantiation of member
>>>>> function 'std::__1::pair<llvm::Regex, const char *>::pair' requested here
>>>>>              ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
>>>>>                                ^
>>>>> /usr/include/c++/v1/memory:1658:18: note: in instantiation of function
>>>>> template specialization 'std::__1::allocator<std::__1::pair<llvm::Regex,
>>>>> const char *> >::construct<std::__1::pair<llvm::Regex, const char *>,
>>>>> const
>>>>> std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>>              {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}
>>>>>                   ^
>>>>> /usr/include/c++/v1/memory:1504:14: note: in instantiation of function
>>>>> template specialization
>>>>> 
>>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>>> const char *> > >::__construct<std::__1::pair<llvm::Regex, const char *>,
>>>>> const std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>>              {__construct(__has_construct<allocator_type, _Tp*,
>>>>> _Args...>(),
>>>>>               ^
>>>>> /usr/include/c++/v1/memory:1620:17: note: in instantiation of function
>>>>> template specialization
>>>>> 
>>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>>> const char *> > >::construct<std::__1::pair<llvm::Regex, const char *>,
>>>>> const std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>>                  construct(__a, _VSTD::__to_raw_pointer(__end2-1),
>>>>> _VSTD::move_if_noexcept(*--__end1));
>>>>>                  ^
>>>>> /usr/include/c++/v1/vector:892:21: note: in instantiation of function
>>>>> template specialization
>>>>> 
>>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>>> const char *> > >::__construct_backward<std::__1::pair<llvm::Regex, const
>>>>> char *> *>' requested here
>>>>>      __alloc_traits::__construct_backward(this->__alloc(),
>>>>> this->__begin_,
>>>>> this->__end_, __v.__begin_);
>>>>>                      ^
>>>>> /usr/include/c++/v1/vector:1537:9: note: in instantiation of member
>>>>> function
>>>>> 'std::__1::vector<std::__1::pair<llvm::Regex, const char *>,
>>>>> std::__1::allocator<std::__1::pair<llvm::Regex, const char *> >
>>>>>> 
>>>>>> ::__swap_out_circular_buffer' requested here
>>>>> 
>>>>>          __swap_out_circular_buffer(__v);
>>>>>          ^
>>>>> 
>>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp:19:33:
>>>>> note: in instantiation of member function
>>>>> 'std::__1::vector<std::__1::pair<llvm::Regex, const char *>,
>>>>> std::__1::allocator<std::__1::pair<llvm::Regex, const char *> >
>>>>>> ::reserve'
>>>>> requested here
>>>>> 
>>>>> this->RegexHeaderMappingTable.reserve(RegexHeaderMappingTable->size());
>>>>>                                  ^
>>>>> /w/src/llvm.org/include/llvm/Support/Regex.h:49:5: note: 'Regex' has been
>>>>> explicitly marked deleted here
>>>>>      Regex(const Regex &) = delete;
>>>>>      ^
>>>>> 1 error generated.
>>>>> *** Error code 1
>>>>> 
>>>>> 
>>>>> -Krzysztof
>>>>> 
>>>>> 
>>>>> 
>>>>> On 11/22/2017 9:38 AM, Benjamin Kramer via cfe-commits wrote:
>>>>>> 
>>>>>> 
>>>>>> Author: d0k
>>>>>> Date: Wed Nov 22 07:38:23 2017
>>>>>> New Revision: 318840
>>>>>> 
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=318840&view=rev
>>>>>> Log:
>>>>>> [FindAllSymbols] Cache regexes, creating them is expensive
>>>>>> 
>>>>>> This is a bit annoying because LLVM regexes are always mutable to store
>>>>>> errors. Assert that there are never errors and fix broken hardcoded
>>>>>> regexes.
>>>>>> 
>>>>>> Modified:
>>>>>> 
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>>> 
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>>> 
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>>> 
>>>>>> Modified:
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>>> URL:
>>>>>> 
>>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp?rev=318840&r1=318839&r2=318840&view=diff
>>>>>> 
>>>>>> 
>>>>>> ==============================================================================
>>>>>> ---
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>>> (original)
>>>>>> +++
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>>> Wed Nov 22 07:38:23 2017
>>>>>> @@ -13,6 +13,16 @@
>>>>>>    namespace clang {
>>>>>>    namespace find_all_symbols {
>>>>>>    +HeaderMapCollector::HeaderMapCollector(
>>>>>> +    const RegexHeaderMap *RegexHeaderMappingTable) {
>>>>>> +  assert(RegexHeaderMappingTable);
>>>>>> +
>>>>>> this->RegexHeaderMappingTable.reserve(RegexHeaderMappingTable->size());
>>>>>> +  for (const auto &Entry : *RegexHeaderMappingTable) {
>>>>>> +
>>>>>> this->RegexHeaderMappingTable.emplace_back(llvm::Regex(Entry.first),
>>>>>> +                                               Entry.second);
>>>>>> +  }
>>>>>> +}
>>>>>> +
>>>>>>    llvm::StringRef
>>>>>>    HeaderMapCollector::getMappedHeader(llvm::StringRef Header) const {
>>>>>>      auto Iter = HeaderMappingTable.find(Header);
>>>>>> @@ -20,11 +30,13 @@ HeaderMapCollector::getMappedHeader(llvm
>>>>>>        return Iter->second;
>>>>>>      // If there is no complete header name mapping for this header,
>>>>>> check
>>>>>> the
>>>>>>      // regex header mapping.
>>>>>> -  if (RegexHeaderMappingTable) {
>>>>>> -    for (const auto &Entry : *RegexHeaderMappingTable) {
>>>>>> -      if (llvm::Regex(Entry.first).match(Header))
>>>>>> -        return Entry.second;
>>>>>> -    }
>>>>>> +  for (auto &Entry : RegexHeaderMappingTable) {
>>>>>> +#ifndef NDEBUG
>>>>>> +    std::string Dummy;
>>>>>> +    assert(Entry.first.isValid(Dummy) && "Regex should never be
>>>>>> invalid!");
>>>>>> +#endif
>>>>>> +    if (Entry.first.match(Header))
>>>>>> +      return Entry.second;
>>>>>>      }
>>>>>>      return Header;
>>>>>>    }
>>>>>> 
>>>>>> Modified:
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>>> URL:
>>>>>> 
>>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h?rev=318840&r1=318839&r2=318840&view=diff
>>>>>> 
>>>>>> 
>>>>>> ==============================================================================
>>>>>> ---
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>>> (original)
>>>>>> +++
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>>> Wed Nov 22 07:38:23 2017
>>>>>> @@ -25,10 +25,8 @@ public:
>>>>>>      typedef llvm::StringMap<std::string> HeaderMap;
>>>>>>      typedef std::vector<std::pair<const char *, const char *>>
>>>>>> RegexHeaderMap;
>>>>>>    -  HeaderMapCollector() : RegexHeaderMappingTable(nullptr) {}
>>>>>> -
>>>>>> -  explicit HeaderMapCollector(const RegexHeaderMap
>>>>>> *RegexHeaderMappingTable)
>>>>>> -      : RegexHeaderMappingTable(RegexHeaderMappingTable) {}
>>>>>> +  HeaderMapCollector() = default;
>>>>>> +  explicit HeaderMapCollector(const RegexHeaderMap
>>>>>> *RegexHeaderMappingTable);
>>>>>>        void addHeaderMapping(llvm::StringRef OrignalHeaderPath,
>>>>>>                            llvm::StringRef MappingHeaderPath) {
>>>>>> @@ -47,8 +45,10 @@ private:
>>>>>>      HeaderMap HeaderMappingTable;
>>>>>>        // A map from header patterns to header names.
>>>>>> -  // This is a reference to a hard-coded map.
>>>>>> -  const RegexHeaderMap *const RegexHeaderMappingTable;
>>>>>> +  // The header names are not owned. This is only threadsafe because
>>>>>> the
>>>>>> regexes
>>>>>> +  // never fail.
>>>>>> +  mutable std::vector<std::pair<llvm::Regex, const char *>>
>>>>>> +      RegexHeaderMappingTable;
>>>>>>    };
>>>>>>      } // namespace find_all_symbols
>>>>>> 
>>>>>> Modified:
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>>> URL:
>>>>>> 
>>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp?rev=318840&r1=318839&r2=318840&view=diff
>>>>>> 
>>>>>> 
>>>>>> ==============================================================================
>>>>>> ---
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>>> (original)
>>>>>> +++
>>>>>> 
>>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>>> Wed Nov 22 07:38:23 2017
>>>>>> @@ -314,10 +314,10 @@ const HeaderMapCollector::RegexHeaderMap
>>>>>>          {"include/xlocale.h$", "<cstring>"},
>>>>>>          {"bits/atomic_word.h$", "<memory>"},
>>>>>>          {"bits/basic_file.h$", "<fstream>"},
>>>>>> -      {"bits/c++allocator.h$", "<string>"},
>>>>>> -      {"bits/c++config.h$", "<iosfwd>"},
>>>>>> -      {"bits/c++io.h$", "<ios>"},
>>>>>> -      {"bits/c++locale.h$", "<locale>"},
>>>>>> +      {"bits/c\\+\\+allocator.h$", "<string>"},
>>>>>> +      {"bits/c\\+\\+config.h$", "<iosfwd>"},
>>>>>> +      {"bits/c\\+\\+io.h$", "<ios>"},
>>>>>> +      {"bits/c\\+\\+locale.h$", "<locale>"},
>>>>>>          {"bits/cpu_defines.h$", "<iosfwd>"},
>>>>>>          {"bits/ctype_base.h$", "<locale>"},
>>>>>>          {"bits/cxxabi_tweaks.h$", "<cxxabi.h>"},
>>>>>> 
>>>>>> 
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>> 
>>>>>> 
>>>>>> ---
>>>>>> This email has been checked for viruses by AVG.
>>>>>> http://www.avg.com
>>>>>> 
>>>>> 
>>> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 223 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171125/bfa4a43f/attachment.sig>

More information about the cfe-commits mailing list