Fix a couple of mangling canonicalizer corner case bugs.

[rsmith]

Summary

The hash computed for an ArrayType was different when first constructed
versus when later profiled due to the constructor default argument, and
we were not tracking constructor / destructor variant as part of the
mangled name AST, leading to incorrect equivalences.

[erik.pilkington]

Huh. I think there may be some more (many?) similar bugs to this. Possible testcase: _Z1fI1SEiS0_T_ and _Z1fI1SEiS0_S0_. I think these will be considered equivalent to the canonicalizer because their representation is structurally equivalent, but they are in fact distinct. I think we can fix this with extra indirection. Finding and fixing these as-needed is probably fine, but something to keep in mind.

Otherwise, LGTM!

[rsmith]

In D51463#1223631, @erik.pilkington wrote:

I think there may be some more (many?) similar bugs to this. Possible testcase: _Z1fI1SEiS0_T_ and _Z1fI1SEiS0_S0_. I think these will be considered equivalent to the canonicalizer because their representation is structurally equivalent, but they are in fact distinct.

Yes, you're right; I think we should be preserving the fact that the type was named via a reference to a specific template parameter in the AST, since that's part of the semantics of the mangled name, and clients that aren't demanglers will sometimes care about the difference. (Also, I would like the demangler to have a mode where it /doesn't/ substitute template parameters for their values, as that's basically never what I personally want when demangling a name.)

Doing that has some complications: if we say that 1XIiE and 1YIiE are the same, does that mean that 1YIT_E is in the same equivalence class too, in the case where T_ is i? (I think the answer should be "no", but this does not seem obvious.)

The good news is that this patch is sufficient to ensure that all interesting function and variable manglings in at least clang and llvm demangle to non-equivalent trees. (We still have duplicate ASTs for virtual function thunks, but I'm certainly happy to defer addressing that until we have a need.)

llvm/trunk/include/llvm/Demangle/ItaniumDemangle.h

public:
   }
 };
 
 class ArrayType final : public Node {
   const Node *Base;
   NodeOrString Dimension;
 
 public:
-  ArrayType(const Node *Base_, NodeOrString Dimension_ = NodeOrString())
+  ArrayType(const Node *Base_, NodeOrString Dimension_)
       : Node(KArrayType,
              /*RHSComponentCache=*/Cache::Yes,
              /*ArrayCache=*/Cache::Yes),
         Base(Base_), Dimension(Dimension_) {}
 
   template<typename Fn> void match(Fn F) const { F(Base, Dimension); }
 
   bool hasRHSComponentSlow(OutputStream &) const override { return true; }
case SpecialSubKind::iostream:
       break;
     }
   }
 };
 
 class CtorDtorName final : public Node {
   const Node *Basename;
   const bool IsDtor;
+  const int Variant;
 
 public:
-  CtorDtorName(const Node *Basename_, bool IsDtor_)
-      : Node(KCtorDtorName), Basename(Basename_), IsDtor(IsDtor_) {}
+  CtorDtorName(const Node *Basename_, bool IsDtor_, int Variant_)
+      : Node(KCtorDtorName), Basename(Basename_), IsDtor(IsDtor_),
+        Variant(Variant_) {}
 
-  template<typename Fn> void match(Fn F) const { F(Basename, IsDtor); }
+  template<typename Fn> void match(Fn F) const { F(Basename, IsDtor, Variant); }
 
   void printLeft(OutputStream &S) const override {
     if (IsDtor)
       S += "~";
     S += Basename->getBaseName();
   }
 };
 
default:
       break;
     }
   }
 
   if (consumeIf('C')) {
     bool IsInherited = consumeIf('I');
     if (look() != '1' && look() != '2' && look() != '3' && look() != '5')
       return nullptr;
+    int Variant = look() - '0';
     ++First;
     if (State) State->CtorDtorConversion = true;
     if (IsInherited) {
       if (parseName(State) == nullptr)
         return nullptr;
     }
-    return make<CtorDtorName>(SoFar, false);
+    return make<CtorDtorName>(SoFar, false, Variant);
   }
 
   if (look() == 'D' &&
       (look(1) == '0' || look(1) == '1' || look(1) == '2' || look(1) == '5')) {
+    int Variant = look(1) - '0';
     First += 2;
     if (State) State->CtorDtorConversion = true;
-    return make<CtorDtorName>(SoFar, true);
+    return make<CtorDtorName>(SoFar, true, Variant);
   }
 
   return nullptr;
 }
 
 // <nested-name> ::= N [<CV-Qualifiers>] [<ref-qualifier>] <prefix> <unqualified-name> E
 //               ::= N [<CV-Qualifiers>] [<ref-qualifier>] <template-prefix> <template-args> E
 //

 }
 
 // <array-type> ::= A <positive dimension number> _ <element type>
 //              ::= A [<dimension expression>] _ <element type>
 template<typename Alloc> Node *Db<Alloc>::parseArrayType() {
   if (!consumeIf('A'))
     return nullptr;
 
+  NodeOrString Dimension;
+
   if (std::isdigit(look())) {
-    StringView Dimension = parseNumber();
+    Dimension = parseNumber();
     if (!consumeIf('_'))
       return nullptr;
-    Node *Ty = parseType();
-    if (Ty == nullptr)
-      return nullptr;
-    return make<ArrayType>(Ty, Dimension);
-  }
-
-  if (!consumeIf('_')) {
+  } else if (!consumeIf('_')) {
     Node *DimExpr = parseExpr();
     if (DimExpr == nullptr)
       return nullptr;
     if (!consumeIf('_'))
       return nullptr;
-    Node *ElementType = parseType();
-    if (ElementType == nullptr)
-      return nullptr;
-    return make<ArrayType>(ElementType, DimExpr);
+    Dimension = DimExpr;
   }
 
   Node *Ty = parseType();
   if (Ty == nullptr)
     return nullptr;
-  return make<ArrayType>(Ty);
+  return make<ArrayType>(Ty, Dimension);
 }
 
 // <pointer-to-member-type> ::= M <class type> <member type>
 template<typename Alloc> Node *Db<Alloc>::parsePointerToMemberType() {
   if (!consumeIf('M'))
     return nullptr;
   Node *ClassType = parseType();
   if (ClassType == nullptr)

llvm/trunk/unittests/Support/ItaniumManglingCanonicalizerTest.cpp

return {
     {
       {
         {FragmentKind::Name, "3std", "St"},
         {FragmentKind::Name, "1X", "1Y"},
         {FragmentKind::Name, "N1X1ZE", "N1Y1ZE"},
       },
       {}
     },
+
+    // Check that ctor and dtor variants are considered distinct.
+    {
+      {},
+      {{"_ZN1XC1Ev"}, {"_ZN1XC2Ev"}, {"_ZN1XD1Ev"}, {"_ZN1XD2Ev"}}
+    },
+
+    // Ensure array types with and without bounds are handled properly.
+    {
+      {
+        {FragmentKind::Type, "A_i", "A1_f"},
+      },
+      {
+        {"_Z1fRA_i", "_Z1fRA_i", "_Z1fRA1_f"},
+        {"_Z1fRA1_i"}, {"_Z1fRA_f"},
+      }
+    },
   };
 }
 
 // A function to get a set of test cases for forward template references.
 static std::vector<Testcase> getForwardTemplateReferenceTestcases() {
   return {
     // ForwardTemplateReference does not support canonicalization.
     // FIXME: We should consider ways of fixing this, perhaps by eliminating