[demangler][NFC] OperatorInfo table unit test

[urnathan]

Summary

Placing a run-once test inside the operator lookup function caused problems with the thread sanitizer. See D122975.

D123158 attempted to address the warning with an atomic once-only approach, but missed the libcxxabi use.

This break out the operator table into a member variable, and move the test to the unit test machinery.

I have verify the unit test triggers, if you do misorder the table.

(The table could be shared across instantiations of the demangler, but that requires either inline-vars (a c++17 thing), or a separate object file.)

[JDevlieghere]

Thanks for following up on this Nathan! No concerns from me, but I probably shouldn't be the one accepting this patch.

[dblaikie]

Would a more direct fix be, rather than using the C++ library call_once feature, instead using the equivalent language feature:

static int unused = [&]() {
  /* whatever you want to do once */
}, 0;

Though if we can move the logic out of the production codepath and into a test entirely, that does seem better anyway - so this is probably the right direction. (I probably won't be the one to approve this - maybe @JDevlieghere has a bit more context in this chain of patches)

[urnathan]

um, it seems that although both @JDevlieghere and @dblaikie like the direction of this patch, neither wishes to commit. pretty please?

[dblaikie]

Oh, sorry I missed that Jonas had also passed on this. I'll give it another look (might be later this week though) with an eye for approval

[JDevlieghere]

To clarify: I think this change looks perfectly fine. The only reason I didn't want to accept this is because I'm not at all familiar with libc++. Case in point: I accepted D123158 which turned out not to be the right thing :-)

[llunak]

The change looks good to me too, if that counts as anything from an outsider. But as an outsider I think you LLVM folks tend to overdo the perfectionism while reviewing. There's rather obviously nothing visibly wrong with the change, the chance it'll break something is extremely low, you apparently know this code, and you pushing this should be fine even according to the policy (the "likely-community-consensus" part) rather than blocking on somebody who apparently has a long enough review queue and could be instead reviewing changes that actually need it (*cough* D122974 *cough*).

In D123390#3444623, @JDevlieghere wrote:

To clarify: I think this change looks perfectly fine. The only reason I didn't want to accept this is because I'm not at all familiar with libc++. Case in point: I accepted D123158 which turned out not to be the right thing :-)

Case in point: D123158 would not have actually broken anything, so it's not a wrong thing either :).

[iains]

In D123390#3444794, @llunak wrote:

The change looks good to me too, if that counts as anything from an outsider. But as an outsider I think you LLVM folks tend to overdo the perfectionism while reviewing. There's rather obviously nothing visibly wrong with the change, the chance it'll break something is extremely low, you apparently know this code, and you pushing this should be fine even according to the policy (the "likely-community-consensus" part)

+1 from me on all counts (as another person not familiar with the details of current libc++).

[urnathan]

In D123390#3444623, @JDevlieghere wrote:

To clarify: I think this change looks perfectly fine. The only reason I didn't want to accept this is because I'm not at all familiar with libc++. Case in point: I accepted D123158 which turned out not to be the right thing :-)

I understand and agree with your reasoning, I intended no blame and am sorry if it came across that way.

[JDevlieghere]

In D123390#3445303, @urnathan wrote:

In D123390#3444623, @JDevlieghere wrote:

To clarify: I think this change looks perfectly fine. The only reason I didn't want to accept this is because I'm not at all familiar with libc++. Case in point: I accepted D123158 which turned out not to be the right thing :-)

I understand and agree with your reasoning, I intended no blame and am sorry if it came across that way.

Not at all!

[dblaikie]

Thanks!

libcxxabi/src/demangle/ItaniumDemangle.h

StringView getSymbol() const {
       }
       return Res;
     }
     StringView getName() const { return Name; }
     OIKind getKind() const { return Kind; }
     bool getFlag() const { return Flag; }
     Node::Prec getPrecedence() const { return Prec; }
   };
+  static const OperatorInfo Ops[];
+  static const size_t NumOps;
   const OperatorInfo *parseOperatorEncoding();
 
   /// Parse the <unresolved-name> production.
   Node *parseUnresolvedName(bool Global);
   Node *parseSimpleId();
   Node *parseBaseUnresolvedName();
   Node *parseUnresolvedType();
   Node *parseDestructorName();
if (numLeft() < Length || Length == 0)
     return nullptr;
   StringView Name(First, First + Length);
   First += Length;
   if (Name.startsWith("_GLOBAL__N"))
     return make<NameType>("(anonymous namespace)");
   return make<NameType>(Name);
 }
 
-// If the next 2 chars are an operator encoding, consume them and return their
-// OperatorInfo.  Otherwise return nullptr.
+// Operator encodings
 template <typename Derived, typename Alloc>
-const typename AbstractManglingParser<Derived, Alloc>::OperatorInfo *
-AbstractManglingParser<Derived, Alloc>::parseOperatorEncoding() {
-  static const OperatorInfo Ops[] = {
-      // Keep ordered by encoding
-      {"aN", OperatorInfo::Binary, false, Node::Prec::Assign, "operator&="},
-      {"aS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator="},
-      {"aa", OperatorInfo::Binary, false, Node::Prec::AndIf, "operator&&"},
-      {"ad", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator&"},
-      {"an", OperatorInfo::Binary, false, Node::Prec::And, "operator&"},
-      {"at", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary,
-       "alignof "},
-      {"aw", OperatorInfo::NameOnly, false, Node::Prec::Primary,
-       "operator co_await"},
-      {"az", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary,
-       "alignof "},
-      {"cc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "const_cast"},
-      {"cl", OperatorInfo::Call, false, Node::Prec::Postfix, "operator()"},
-      {"cm", OperatorInfo::Binary, false, Node::Prec::Comma, "operator,"},
-      {"co", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator~"},
-      {"cv", OperatorInfo::CCast, false, Node::Prec::Cast,
-       "operator"}, // C Cast
-      {"dV", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/="},
-      {"da", OperatorInfo::Del, /*Ary*/ true, Node::Prec::Unary,
-       "operator delete[]"},
-      {"dc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "dynamic_cast"},
-      {"de", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator*"},
-      {"dl", OperatorInfo::Del, /*Ary*/ false, Node::Prec::Unary,
-       "operator delete"},
-      {"ds", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
-       "operator.*"},
-      {"dt", OperatorInfo::Member, /*Named*/ false, Node::Prec::Postfix,
-       "operator."},
-      {"dv", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/"},
-      {"eO", OperatorInfo::Binary, false, Node::Prec::Assign, "operator^="},
-      {"eo", OperatorInfo::Binary, false, Node::Prec::Xor, "operator^"},
-      {"eq", OperatorInfo::Binary, false, Node::Prec::Equality, "operator=="},
-      {"ge", OperatorInfo::Binary, false, Node::Prec::Relational, "operator>="},
-      {"gt", OperatorInfo::Binary, false, Node::Prec::Relational, "operator>"},
-      {"ix", OperatorInfo::Array, false, Node::Prec::Postfix, "operator[]"},
-      {"lS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator<<="},
-      {"le", OperatorInfo::Binary, false, Node::Prec::Relational, "operator<="},
-      {"ls", OperatorInfo::Binary, false, Node::Prec::Shift, "operator<<"},
-      {"lt", OperatorInfo::Binary, false, Node::Prec::Relational, "operator<"},
-      {"mI", OperatorInfo::Binary, false, Node::Prec::Assign, "operator-="},
-      {"mL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator*="},
-      {"mi", OperatorInfo::Binary, false, Node::Prec::Additive, "operator-"},
-      {"ml", OperatorInfo::Binary, false, Node::Prec::Multiplicative,
-       "operator*"},
-      {"mm", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator--"},
-      {"na", OperatorInfo::New, /*Ary*/ true, Node::Prec::Unary,
-       "operator new[]"},
-      {"ne", OperatorInfo::Binary, false, Node::Prec::Equality, "operator!="},
-      {"ng", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator-"},
-      {"nt", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator!"},
-      {"nw", OperatorInfo::New, /*Ary*/ false, Node::Prec::Unary,
-       "operator new"},
-      {"oR", OperatorInfo::Binary, false, Node::Prec::Assign, "operator|="},
-      {"oo", OperatorInfo::Binary, false, Node::Prec::OrIf, "operator||"},
-      {"or", OperatorInfo::Binary, false, Node::Prec::Ior, "operator|"},
-      {"pL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator+="},
-      {"pl", OperatorInfo::Binary, false, Node::Prec::Additive, "operator+"},
-      {"pm", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
-       "operator->*"},
-      {"pp", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator++"},
-      {"ps", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator+"},
-      {"pt", OperatorInfo::Member, /*Named*/ true, Node::Prec::Postfix,
-       "operator->"},
-      {"qu", OperatorInfo::Conditional, false, Node::Prec::Conditional,
-       "operator?"},
-      {"rM", OperatorInfo::Binary, false, Node::Prec::Assign, "operator%="},
-      {"rS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator>>="},
-      {"rc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "reinterpret_cast"},
-      {"rm", OperatorInfo::Binary, false, Node::Prec::Multiplicative,
-       "operator%"},
-      {"rs", OperatorInfo::Binary, false, Node::Prec::Shift, "operator>>"},
-      {"sc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "static_cast"},
-      {"ss", OperatorInfo::Binary, false, Node::Prec::Spaceship, "operator<=>"},
-      {"st", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary, "sizeof "},
-      {"sz", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary,
-       "sizeof "},
-      {"te", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Postfix,
-       "typeid "},
-      {"ti", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Postfix,
-       "typeid "},
-  };
-  const auto NumOps = sizeof(Ops) / sizeof(Ops[0]);
-
-#ifndef NDEBUG
-  {
-    // Verify table order.
-    static bool Done;
-    if (!Done) {
-      Done = true;
-      for (const auto *Op = &Ops[0]; Op != &Ops[NumOps - 1]; Op++)
-        assert(Op[0] < Op[1] && "Operator table is not ordered");
-    }
-  }
-#endif
+const typename AbstractManglingParser<
+    Derived, Alloc>::OperatorInfo AbstractManglingParser<Derived,
+                                                         Alloc>::Ops[] = {
+    {"at", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary, "alignof "},
+    {"az", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary, "alignof "},
+    {"cv", OperatorInfo::CCast, false, Node::Prec::Cast, "operator"}, // C Cast
+    {"dc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "dynamic_cast"},
+    {"nw", OperatorInfo::New, /*Ary*/ false, Node::Prec::Unary, "operator new"},
+    {"sc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "static_cast"},
+    {"sz", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary, "sizeof "},
+    {"ti", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Postfix, "typeid "},
+template <typename Derived, typename Alloc>
+const size_t AbstractManglingParser<Derived, Alloc>::NumOps = sizeof(Ops) /
+                                                              sizeof(Ops[0]);
 
+// If the next 2 chars are an operator encoding, consume them and return their
+// OperatorInfo.  Otherwise return nullptr.
+template <typename Derived, typename Alloc>
+const typename AbstractManglingParser<Derived, Alloc>::OperatorInfo *
+AbstractManglingParser<Derived, Alloc>::parseOperatorEncoding() {
   if (numLeft() < 2)
     return nullptr;
 
   auto Op = std::lower_bound(
       &Ops[0], &Ops[NumOps], First,
       [](const OperatorInfo &Op_, const char *Enc_) { return Op_ < Enc_; });
   if (Op == &Ops[NumOps] || *Op != First)
     return nullptr;

llvm/include/llvm/Demangle/ItaniumDemangle.h

StringView getSymbol() const {
       }
       return Res;
     }
     StringView getName() const { return Name; }
     OIKind getKind() const { return Kind; }
     bool getFlag() const { return Flag; }
     Node::Prec getPrecedence() const { return Prec; }
   };
+  static const OperatorInfo Ops[];
+  static const size_t NumOps;
   const OperatorInfo *parseOperatorEncoding();
 
   /// Parse the <unresolved-name> production.
   Node *parseUnresolvedName(bool Global);
   Node *parseSimpleId();
   Node *parseBaseUnresolvedName();
   Node *parseUnresolvedType();
   Node *parseDestructorName();
if (numLeft() < Length || Length == 0)
     return nullptr;
   StringView Name(First, First + Length);
   First += Length;
   if (Name.startsWith("_GLOBAL__N"))
     return make<NameType>("(anonymous namespace)");
   return make<NameType>(Name);
 }
 
-// If the next 2 chars are an operator encoding, consume them and return their
-// OperatorInfo.  Otherwise return nullptr.
+// Operator encodings
 template <typename Derived, typename Alloc>
-const typename AbstractManglingParser<Derived, Alloc>::OperatorInfo *
-AbstractManglingParser<Derived, Alloc>::parseOperatorEncoding() {
-  static const OperatorInfo Ops[] = {
-      // Keep ordered by encoding
-      {"aN", OperatorInfo::Binary, false, Node::Prec::Assign, "operator&="},
-      {"aS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator="},
-      {"aa", OperatorInfo::Binary, false, Node::Prec::AndIf, "operator&&"},
-      {"ad", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator&"},
-      {"an", OperatorInfo::Binary, false, Node::Prec::And, "operator&"},
-      {"at", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary,
-       "alignof "},
-      {"aw", OperatorInfo::NameOnly, false, Node::Prec::Primary,
-       "operator co_await"},
-      {"az", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary,
-       "alignof "},
-      {"cc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "const_cast"},
-      {"cl", OperatorInfo::Call, false, Node::Prec::Postfix, "operator()"},
-      {"cm", OperatorInfo::Binary, false, Node::Prec::Comma, "operator,"},
-      {"co", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator~"},
-      {"cv", OperatorInfo::CCast, false, Node::Prec::Cast,
-       "operator"}, // C Cast
-      {"dV", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/="},
-      {"da", OperatorInfo::Del, /*Ary*/ true, Node::Prec::Unary,
-       "operator delete[]"},
-      {"dc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "dynamic_cast"},
-      {"de", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator*"},
-      {"dl", OperatorInfo::Del, /*Ary*/ false, Node::Prec::Unary,
-       "operator delete"},
-      {"ds", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
-       "operator.*"},
-      {"dt", OperatorInfo::Member, /*Named*/ false, Node::Prec::Postfix,
-       "operator."},
-      {"dv", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/"},
-      {"eO", OperatorInfo::Binary, false, Node::Prec::Assign, "operator^="},
-      {"eo", OperatorInfo::Binary, false, Node::Prec::Xor, "operator^"},
-      {"eq", OperatorInfo::Binary, false, Node::Prec::Equality, "operator=="},
-      {"ge", OperatorInfo::Binary, false, Node::Prec::Relational, "operator>="},
-      {"gt", OperatorInfo::Binary, false, Node::Prec::Relational, "operator>"},
-      {"ix", OperatorInfo::Array, false, Node::Prec::Postfix, "operator[]"},
-      {"lS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator<<="},
-      {"le", OperatorInfo::Binary, false, Node::Prec::Relational, "operator<="},
-      {"ls", OperatorInfo::Binary, false, Node::Prec::Shift, "operator<<"},
-      {"lt", OperatorInfo::Binary, false, Node::Prec::Relational, "operator<"},
-      {"mI", OperatorInfo::Binary, false, Node::Prec::Assign, "operator-="},
-      {"mL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator*="},
-      {"mi", OperatorInfo::Binary, false, Node::Prec::Additive, "operator-"},
-      {"ml", OperatorInfo::Binary, false, Node::Prec::Multiplicative,
-       "operator*"},
-      {"mm", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator--"},
-      {"na", OperatorInfo::New, /*Ary*/ true, Node::Prec::Unary,
-       "operator new[]"},
-      {"ne", OperatorInfo::Binary, false, Node::Prec::Equality, "operator!="},
-      {"ng", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator-"},
-      {"nt", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator!"},
-      {"nw", OperatorInfo::New, /*Ary*/ false, Node::Prec::Unary,
-       "operator new"},
-      {"oR", OperatorInfo::Binary, false, Node::Prec::Assign, "operator|="},
-      {"oo", OperatorInfo::Binary, false, Node::Prec::OrIf, "operator||"},
-      {"or", OperatorInfo::Binary, false, Node::Prec::Ior, "operator|"},
-      {"pL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator+="},
-      {"pl", OperatorInfo::Binary, false, Node::Prec::Additive, "operator+"},
-      {"pm", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
-       "operator->*"},
-      {"pp", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator++"},
-      {"ps", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator+"},
-      {"pt", OperatorInfo::Member, /*Named*/ true, Node::Prec::Postfix,
-       "operator->"},
-      {"qu", OperatorInfo::Conditional, false, Node::Prec::Conditional,
-       "operator?"},
-      {"rM", OperatorInfo::Binary, false, Node::Prec::Assign, "operator%="},
-      {"rS", OperatorInfo::Binary, false, Node::Prec::Assign, "operator>>="},
-      {"rc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "reinterpret_cast"},
-      {"rm", OperatorInfo::Binary, false, Node::Prec::Multiplicative,
-       "operator%"},
-      {"rs", OperatorInfo::Binary, false, Node::Prec::Shift, "operator>>"},
-      {"sc", OperatorInfo::NamedCast, false, Node::Prec::Postfix,
-       "static_cast"},
-      {"ss", OperatorInfo::Binary, false, Node::Prec::Spaceship, "operator<=>"},
-      {"st", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary, "sizeof "},
-      {"sz", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary,
-       "sizeof "},
-      {"te", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Postfix,
-       "typeid "},
-      {"ti", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Postfix,
-       "typeid "},
-  };
-  const auto NumOps = sizeof(Ops) / sizeof(Ops[0]);
-
-#ifndef NDEBUG
-  {
-    // Verify table order.
-    static bool Done;
-    if (!Done) {
-      Done = true;
-      for (const auto *Op = &Ops[0]; Op != &Ops[NumOps - 1]; Op++)
-        assert(Op[0] < Op[1] && "Operator table is not ordered");
-    }
-  }
-#endif
+const typename AbstractManglingParser<
+    Derived, Alloc>::OperatorInfo AbstractManglingParser<Derived,
+                                                         Alloc>::Ops[] = {
+    {"at", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Unary, "alignof "},
+    {"az", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary, "alignof "},
+    {"cv", OperatorInfo::CCast, false, Node::Prec::Cast, "operator"}, // C Cast
+    {"dc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "dynamic_cast"},
+    {"nw", OperatorInfo::New, /*Ary*/ false, Node::Prec::Unary, "operator new"},
+    {"sc", OperatorInfo::NamedCast, false, Node::Prec::Postfix, "static_cast"},
+    {"sz", OperatorInfo::OfIdOp, /*Type*/ false, Node::Prec::Unary, "sizeof "},
+    {"ti", OperatorInfo::OfIdOp, /*Type*/ true, Node::Prec::Postfix, "typeid "},
+template <typename Derived, typename Alloc>
+const size_t AbstractManglingParser<Derived, Alloc>::NumOps = sizeof(Ops) /
+                                                              sizeof(Ops[0]);
 
+// If the next 2 chars are an operator encoding, consume them and return their
+// OperatorInfo.  Otherwise return nullptr.
+template <typename Derived, typename Alloc>
+const typename AbstractManglingParser<Derived, Alloc>::OperatorInfo *
+AbstractManglingParser<Derived, Alloc>::parseOperatorEncoding() {
   if (numLeft() < 2)
     return nullptr;
 
   auto Op = std::lower_bound(
       &Ops[0], &Ops[NumOps], First,
       [](const OperatorInfo &Op_, const char *Enc_) { return Op_ < Enc_; });
   if (Op == &Ops[NumOps] || *Op != First)
     return nullptr;

llvm/unittests/Demangle/ItaniumDemangleTest.cpp

 #undef NOMATCHER
 
 void Visitor() {
 #define NODE(X) Visit(static_cast<const itanium_demangle::X *>(nullptr));
 #include "llvm/Demangle/ItaniumNodes.def"
 }
 } // namespace NodeMatcher
 
+// Verify Operator table is ordered
+TEST(ItaniumDemangle, OperatorOrdering) {
+  struct TestParser : AbstractManglingParser<TestParser, TestAllocator> {};
+  for (const auto *Op = &TestParser::Ops[0];
+       Op != &TestParser::Ops[TestParser::NumOps - 1]; Op++)
+    ASSERT_LT(Op[0], Op[1]);
+}
+
 TEST(ItaniumDemangle, MethodOverride) {
   struct TestParser : AbstractManglingParser<TestParser, TestAllocator> {
     std::vector<char> Types;
 
     TestParser(const char *Str)
         : AbstractManglingParser(Str, Str + strlen(Str)) {}
 
     Node *parseType() {