[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a reviewer: mizvekov.
mizvekov added inline comments.


================
Comment at: clang/lib/AST/TemplateBase.cpp:373-374
   case TemplateExpansion:
-    return TemplateArg.Name == Other.TemplateArg.Name &&
+    return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+               Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
            TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
----------------
I believe this change is not correct, as here we want to compare these two template arguments to see if they are identical (structural equality), not just that they refer to the same thing.


================
Comment at: clang/test/CXX/dcl/dcl.fct/p17.cpp:261-279
+  template <typename T> struct S3 {};
+  // expected-note at -1 {{'S3' declared here}}
+  // expected-note at -2 {{template is declared here}}
+  // clang-format off
+  void f23(C2<::S3> auto);
+  // expected-error at -1 {{no template named 'S3' in the global namespace; did you mean simply 'S3'?}}
+  // expected-error at -2 {{use of class template '::S3' requires template arguments}}
----------------
So I think that the patch is just papering over the bug with another one of no consequence in these tests.

If you look at this code in ASTContext.cpp around line 5695 on getAutoTypeInternal:
```
      bool AnyNonCanonArgs =
          ::getCanonicalTemplateArguments(*this, TypeConstraintArgs, CanonArgs);
      if (AnyNonCanonArgs) {
        Canon = getAutoTypeInternal(QualType(), Keyword, IsDependent, IsPack,
                                    TypeConstraintConcept, CanonArgs, true);
        // Find the insert position again.
        AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
      }
```
Your patch makes AnyNonCanonArgs be false when it should be true, as structural comparison of the argument to the canonical argument should indicate that the argument is not canonical.

What is happening here on the real case, where `AnyNonCanonArgs = true`, is that we end up outputting a NULL InsertPos on that "// Find the insert position again." part, clearly because that `FindNodeOrInsertPos` actually found the node.

But that should be impossible, because we clearly looked for it earlier and would have returned before if so.

So that means that the call to create the canonical autotype somehow created the same type as we want to create for the non-canonical one, which again is bananas because the arguments are clearly not structurally identical as we established.

So I think the only possibility here is that this is a profiling bug, we are somehow not profiling template arguments correctly.

A quick look at `TemplateArgument::Profile` shows some things that look clearly long, for example in the template case where we profile the canonical template name instead of the as-written one. This is probably the bug that is hitting these test cases.

There is also a problem in the Declaration case as well, where we are taking the canonical declaration, this should presumably also cause a similar bug.

If that does fix it, can you also add tests which would have caught the bug that this current patch would have introduced?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172