string_view equality operator does not compile with clang-cl

[Quuxplusone]

Bugzilla Link	PR50001
Status	NEW
Importance	P normal
Reported by	Hoch Hochkeppel (mhochk@microsoft.com)
Reported on	2021-04-16 11:07:44 -0700
Last modified on	2021-04-20 15:40:36 -0700
Version	unspecified
Hardware	PC Windows NT
CC	llvm-bugs@lists.llvm.org, mclow.lists@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Context first:

There is a name-mangling issue/bug/limitation in clang-cl (doesn't appear to
impact clang directly) easily repro'd by attempting to compile the following
(locally named clang-cl-issue.cpp):

template <class T>
struct X {
  T member;
};
template <class T>
struct ConvertibleToX {
  operator X<T>() { return {T{}}; }
};
template <class T>
struct identity {
  using type = T;
};
template <class T>
bool operator==(const X<T> lhs, const X<T> rhs) {
  return (true);
}
template <class T>
bool operator==(const X<T> lhs, const typename identity<X<T>>::type rhs) {
  return (true);
}
template <class T>
bool operator==(const typename identity<X<T>>::type lhs, const X<T> rhs) {
  return (true);
}
int main() {
  X<int> xVal{42};
  ConvertibleToX<int> xConvVal;
  bool result = xVal == xVal;
  result = xVal == xConvVal;
  result = xConvVal == xVal;
  return result ? 1 : 0;
}

RESULT:
clang-cl-issue.cpp(18,6): error: definition with same mangled name
'??$?8H@@YA_NU?$X@H@@0@Z' as another definition
bool operator==(const X<T> lhs, const typename identity<X<T>>::type rhs) {
     ^
clang-cl-issue.cpp(14,6): note: previous definition is here
bool operator==(const X<T> lhs, const X<T> rhs) {
     ^
clang-cl-issue.cpp(22,6): error: definition with same mangled name
'??$?8H@@YA_NU?$X@H@@0@Z' as another definition
bool operator==(const typename identity<X<T>>::type lhs, const X<T> rhs) {
     ^
clang-cl-issue.cpp(14,6): note: previous definition is here
bool operator==(const X<T> lhs, const X<T> rhs) {

This issue is relevant to the libc++ component because the string_view
implementation currently uses the same pattern, raising the same errors with
clang-cl with very simple usage. For example, attempting to compile:

#include <string>
#include <string_view>
#include <unordered_set>

constexpr std::string_view kSomeView("Hello");

bool TestFunction(
  const std::unordered_set<std::string_view>& telemetry,
  const std::string& s) noexcept
{
  if (telemetry.find(s) != telemetry.end()) {
    return s == kSomeView;
  }
  return false;
}

RESULT:
{MyLocalPath}\libcxx\include\string_view(614,6): error: definition with same
mangled name
      '??$?8DU?$char_traits@D@__1@std@@@__1@std@@YA_NV?$basic_string_view@DU?$char_traits@D@__1@std@@@01@0@Z' as another definition
bool operator==(basic_string_view<_CharT, _Traits> __lhs,
     ^
{MyLocalPath}\libcxx\include\string_view(632,6): note: previous definition is
here
bool operator==(typename common_type<basic_string_view<_CharT, _Traits> >::type
__lhs,
     ^

Though the root issue is with clang-cl, this can be worked around very easily
by adding additional qualifiers to the string_view equality operator templates
to force them to be unique. Added below ", int = 1" and ", int = 2" fixes this
problem for string_view usage.

// [string.view.comparison]
// operator ==
template<class _CharT, class _Traits>
_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
bool operator==(basic_string_view<_CharT, _Traits> __lhs,
                basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
{
    if ( __lhs.size() != __rhs.size()) return false;
    return __lhs.compare(__rhs) == 0;
}

template<class _CharT, class _Traits, int = 1>
_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
bool operator==(basic_string_view<_CharT, _Traits> __lhs,
                typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) _NOEXCEPT
{
    if ( __lhs.size() != __rhs.size()) return false;
    return __lhs.compare(__rhs) == 0;
}

template<class _CharT, class _Traits, int = 2>
_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
bool operator==(typename common_type<basic_string_view<_CharT, _Traits> >::type
__lhs,
                basic_string_view<_CharT, _Traits> __rhs) _NOEXCEPT
{
    if ( __lhs.size() != __rhs.size()) return false;
    return __lhs.compare(__rhs) == 0;
}

[Quuxplusone]

If we were to do something like this, I suspect that we'd have to do it in a LOT of places; not just in string_view.

Why is clang-cl doing this?

[Quuxplusone]

I don't have a complete understanding of *why* clang-cl is doing this, but have
been told and pointed at a Microsoft internal bug #409326 that says this is a
flaw in the name mangling logic and does not work with the "identity_t trick"
(which is a shorthand for this syntax being used). This bug is unfortunately
from before such issues were hosted in a public location so I can't share a
link to it, but I can point at where this same work-around is being applied in
the Microsoft MSVC/STL implementation of string_view here: (Search for 409326)

https://github.com/microsoft/STL/blob/main/stl/inc/xstring

In my search through the libcxx code for a similar pattern the only place I see
overloaded functions relying on 'typename identity/common_type<>::type' to
distinguish them is in the string_view comparison operators (though admittedly
more than just the == operator). I may have missed some instance somewhere as I
only read a subset of the code based on search results, but the fact that the
STL implementation only references this bug in the string_view operators and
the fact that the underlying name mangling bug is 4+ years old without this
issue having come up publicly before encourages me that this is indeed the only
location being impacted.

[Quuxplusone]

When I look at that file you reference, I see that "trick" being done 13 times:

twice on operator==
twice on operator!=
twice on operator<
twice on operator<=
twice on operator>
twice on operator>=
once on operator<=>

[Quuxplusone]

Yes, sorry I was not explicit about that. When I said the only place this applied was the "string_view comparison operators (though admittedly more than just the == operator)" I meant all the comparison operators for string_view, not just the equality operator.