PR15677 - Crash in template diffing.

[nikola]

Summary

In addition to having and integral type the expression needs to be evaluatable.

I'm not too happy with how GetInt function looks... one idea would be to return a bool and take APInt as an output parameter, in that case the function would look something like this:

bool GetInt(const TSTiterator &Iter, Expr *ArgExpr, llvm::APInt &Int) {
  // Default, value-depenedent expressions require fetching
  // from the desugared TemplateArgument
  if (Iter.isEnd() && ArgExpr->isValueDependent()) {
    switch (Iter.getDesugar().getKind()) {
    case TemplateArgument::Integral:
      Int = Iter.getDesugar().getAsIntegral();
    case TemplateArgument::Expression:
      ArgExpr = Iter.getDesugar().getAsExpr();
      Int = ArgExpr->EvaluateKnownConstInt(Context);
    default:
      llvm_unreachable("Unexpected template argument kind");
    }
    return true;
  } else if (ArgExpr->isEvaluatable(Context)) {
    Int = ArgExpr->EvaluateKnownConstInt(Context);
    return true;
  }

  return false;
}

[rtrieu]

That seems rather special. I'm not entirely convinced that all the edge are taken care of here. I will be able to take a closer look at this patch next week.

[rtrieu]

Besides the above, PrintAPSInt needs to updated since a new node format is being introduced. On the else branch where "(no argument)" is printed, this should be changed to print the expression if E is valid, and print "(no argument)" if it is not.

lib/AST/ASTDiagnostic.cpp

970

This needs another case here, (HasFromInt || HasToInt). Use same SetNode and SetKind as above, but use SetSame(false).

1122

Go with version you put in the summary: returns bool and takes APInt as an reference argument. Update the comment here.

test/Misc/diag-template-diffing.cpp

1100

Add another test case, just like E, but use 21 + 21 and test that "21 + 21 aka 42" is printed.

[nikola]

Good catch Richard, I had the feeling that I've missed something... This patch should address issues you raised.

P.S. I have commit access.

[rtrieu]

lib/AST/ASTDiagnostic.cpp

1131–1137

Why did you change from a switch statement to an if chain here?

[nikola]

lib/AST/ASTDiagnostic.cpp

1131–1137

The entire function was coded in Phabricator and was missing breaks. After adding those it seem to be a fair bit longer than needed. Do you want me to change it?

[rtrieu]

LGTM

Address the one comment and you are ready to submit.

lib/AST/ASTDiagnostic.cpp

1131–1137

Yes, please change it back. Also, use "return true;" instead of breaks and remove the "return true;" below. This mirrors the old codeflow better.

[nikola]

r212090. Thanks for the review.

lib/AST/ASTDiagnostic.cpp

           ToExpr = GetExpr(ToIter, DefaultNTTPD);
 
         if (!HasFromInt && !HasToInt && !HasFromValueDecl && !HasToValueDecl) {
           Tree.SetNode(FromExpr, ToExpr);
           Tree.SetDefault(FromIter.isEnd() && FromExpr,
                           ToIter.isEnd() && ToExpr);
           if (DefaultNTTPD->getType()->isIntegralOrEnumerationType()) {
             if (FromExpr)
-              FromInt = GetInt(FromIter, FromExpr);
+              HasFromInt = GetInt(FromIter, FromExpr, FromInt);
             if (ToExpr)
-              ToInt = GetInt(ToIter, ToExpr);
-            Tree.SetNode(FromInt, ToInt, FromExpr, ToExpr);
+              HasToInt = GetInt(ToIter, ToExpr, ToInt);
+          }
+          if (HasFromInt && HasToInt) {
+            Tree.SetNode(FromInt, ToInt, HasFromInt, HasToInt);
             Tree.SetSame(IsSameConvertedInt(ParamWidth, FromInt, ToInt));
             Tree.SetKind(DiffTree::Integer);
+          } else if (HasFromInt || HasToInt) {
+            Tree.SetNode(FromInt, ToInt, HasFromInt, HasToInt);
+            Tree.SetSame(false);
+            Tree.SetKind(DiffTree::Integer);
           } else {
             Tree.SetSame(IsEqualExpr(Context, ParamWidth, FromExpr, ToExpr));
             Tree.SetKind(DiffTree::Expression);
           }
         } else if (HasFromInt || HasToInt) {
-          if (!HasFromInt && FromExpr) {
-            FromInt = GetInt(FromIter, FromExpr);
-            HasFromInt = true;
-          }
-          if (!HasToInt && ToExpr) {
-            ToInt = GetInt(ToIter, ToExpr);
-            HasToInt = true;
-          }
+          if (!HasFromInt && FromExpr)
+            HasFromInt = GetInt(FromIter, FromExpr, FromInt);
+          if (!HasToInt && ToExpr)
+            HasToInt = GetInt(ToIter, ToExpr, ToInt);
           Tree.SetNode(FromInt, ToInt, HasFromInt, HasToInt);
           Tree.SetSame(IsSameConvertedInt(ParamWidth, FromInt, ToInt));
           Tree.SetDefault(FromIter.isEnd() && HasFromInt,
                           ToIter.isEnd() && HasToInt);
           Tree.SetKind(DiffTree::Integer);
         } else {
           if (!HasFromValueDecl && FromExpr)
             FromValueDecl = GetValueDecl(FromIter, FromExpr);

       ArgExpr = DefaultNTTPD->getDefaultArgument();
 
     if (ArgExpr)
       while (SubstNonTypeTemplateParmExpr *SNTTPE =
                  dyn_cast<SubstNonTypeTemplateParmExpr>(ArgExpr))
         ArgExpr = SNTTPE->getReplacement();
 
     return ArgExpr;
   }
 
   /// GetInt - Retrieves the template integer argument, including evaluating
   /// default arguments.
-  llvm::APInt GetInt(const TSTiterator &Iter, Expr *ArgExpr) {
+  bool GetInt(const TSTiterator &Iter, Expr *ArgExpr, llvm::APInt &Int) {
     // Default, value-depenedent expressions require fetching
-    // from the desugared TemplateArgument
-    if (Iter.isEnd() && ArgExpr->isValueDependent())
-      switch (Iter.getDesugar().getKind()) {
-        case TemplateArgument::Integral:
-          return Iter.getDesugar().getAsIntegral();
-        case TemplateArgument::Expression:
-          ArgExpr = Iter.getDesugar().getAsExpr();
-          return ArgExpr->EvaluateKnownConstInt(Context);
-        default:
-          llvm_unreachable("Unexpected template argument kind");
+    // from the desugared TemplateArgument, otherwise expression needs to
+    // be evaluatable.
+    if (Iter.isEnd() && ArgExpr->isValueDependent()) {
+      TemplateArgument::ArgKind Kind = Iter.getDesugar().getKind();
+      if (Kind == TemplateArgument::Integral) {
+        Int = Iter.getDesugar().getAsIntegral();
+      } else if (Kind == TemplateArgument::Expression) {
+        ArgExpr = Iter.getDesugar().getAsExpr();
+        Int = ArgExpr->EvaluateKnownConstInt(Context);
+      } else {
+        llvm_unreachable("Unexpected template argument kind");
       }
-    return ArgExpr->EvaluateKnownConstInt(Context);
+      return true;
+    } else if (ArgExpr->isEvaluatable(Context)) {
+      Int = ArgExpr->EvaluateKnownConstInt(Context);
+      return true;
+    }
+
+    return false;
   }
 
   /// GetValueDecl - Retrieves the template Decl argument, including
   /// default expression argument.
   ValueDecl *GetValueDecl(const TSTiterator &Iter, Expr *ArgExpr) {
     // Default, value-depenedent expressions require fetching
     // from the desugared TemplateArgument
     if (Iter.isEnd() && ArgExpr->isValueDependent())

     if (Valid) {
       if (HasExtraInfo(E)) {
         PrintExpr(E);
         Unbold();
         OS << " aka ";
         Bold();
       }
       OS << Val.toString(10);
+    } else if (E) {
+      PrintExpr(E);
     } else {
       OS << "(no argument)";
     }
     Unbold();
   }
   
   /// HasExtraInfo - Returns true if E is not an integer literal or the
   /// negation of an integer literal

test/Misc/diag-template-diffing.cpp

 
 void foo() {
   vector<Atom *> v;
   AtomVector v2(v);
   // CHECK-ELIDE-NOTREE: no known conversion from 'vector<class PR17510::Atom *, [...]>' to 'const vector<const class PR17510::Atom *, [...]>'
 }
 }
 
+namespace PR15677 {
+template <bool>
+struct A{};
+
+template <typename T>
+using B = A<T::value>;
+
+template <typename T>
+using B = A<!T::value>;
+// CHECK-ELIDE-NOTREE: type alias template redefinition with different types ('A<!T::value>' vs 'A<T::value>')
+
+template <int>
+struct C{};
+
+template <typename T>
+using D = C<T::value>;
+
+template <typename T>
+using D = C<T::value + 1>;
+// CHECK-ELIDE-NOTREE: type alias template redefinition with different types ('C<T::value + 1>' vs 'C<T::value>')
+
+template <typename T>
+using E = C<T::value>;
+
+template <typename T>
+using E = C<42>;
+// CHECK-ELIDE-NOTREE: type alias template redefinition with different types ('C<42>' vs 'C<T::value>')
+
+template <typename T>
+using F = C<T::value>;
+
+template <typename T>
+using F = C<21 + 21>;
+// CHECK-ELIDE-NOTREE: type alias template redefinition with different types ('C<21 + 21 aka 42>' vs 'C<T::value>')
+}
+}
+
 // CHECK-ELIDE-NOTREE: {{[0-9]*}} errors generated.
 // CHECK-NOELIDE-NOTREE: {{[0-9]*}} errors generated.
 // CHECK-ELIDE-TREE: {{[0-9]*}} errors generated.
 // CHECK-NOELIDE-TREE: {{[0-9]*}} errors generated.