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

[roberteg16]

Summary

This diff path takes care of comparing Template{Expansion} by template decl instead of by comparing the ptr memberName and the number of expansions.

Hi! This is my first attempt to summit a differential revision probably I did something wrong. Someone pointing out the errors would be highly appreciated.

A few days ago I uploaded a crash on clang. The following patch is an attempt to fix it, I am not very familiar with this part of Clang so probably a lot of things went over my head.

The crash happens when after parsing a declarator clang tries to get its type. In this case, is for an auto declarator when concepts are involved, this happens on clang::ASTContext::getAutoTypeInternal. It seems that when clang tries to retrieve the canonical arguments, it fails since the non-canonical and the canonical do not match because (I think) they way they are compared.

This makes the if statement to be evaluated to false and therefore InsertPos to be nullptr. This ends in a crash when line 5721 is reached, and a nullptr being inserted.

I also included some test, but I am not sure I placed them in the best place. let me know if there is a better place.

Thanks for your time.

[roberteg16]

Make clang-format not complain

[roberteg16]

Stricten comparison

[roberteg16]

Try fix clang-format

[roberteg16]

Fix clang-format

[roberteg16]

Fix wrongly moved clang-format disabling comment when trying to fix clang-format

[roberteg16]

@rsmith polite ping

[mizvekov]

clang/lib/AST/TemplateBase.cpp

373–375

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.

clang/test/CXX/dcl/dcl.fct/p17.cpp

261–279

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?

[roberteg16]

Thanks @mizvekov for such an insightful review. I'll try to address ASAP everything you said and let you know the results.

[roberteg16]

Hi again @mizvekov, I've been spending a bit of time on resuming this issue.

When applying (after reverting the changes) the first change related to the Template case of profiling the as-written one name this resulted in the following test failing:

Changes:

-ID.AddPointer(Context.getCanonicalTemplateName(Template)
                                                    .getAsVoidPointer());
+ID.AddPointer(Template.getAsVoidPointer());

Test failing:

Clang :: CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
Clang :: CXX/temp/temp.decls/temp.variadic/multi-level-substitution.cpp
Clang :: CodeGenCXX/mangle-variadic-templates.cpp
Clang :: CodeGenCXX/mangle.cpp

The main reason of the failure is that now clang outputs errors. I'll only copy the CodeGenCXX/mangle.cpp output:

/home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:516:17: error: explicit instantiation of 'foo' does not refer to a function template, variable template, member function, member class, or static data member
  template void foo(const A<B> &a);
                ^
/home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:513:43: note: candidate template ignored: failed template argument deduction
  template <template<class> class T> void foo(const A<T> &a) {}
                                          ^
/home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:1146:10: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
    auto [e] = g;
         ^~~
/home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:1153:8: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
  auto [a,b] = X{1,2};

I've been looking around trying to figure out what's the issue but I am feeling that I lack the required knowledge to diagnose properly the problem.

One question I want to ask is: On StmtProfiler::VisitTemplateArgument it states on a comment:

// Mostly repetitive with TemplateArgument::Profile!

but as far as I could see it really does not mimic the behavior in some cases. Is this at purpose?

Thanks for your time.

[mizvekov]

In D126172#3626809, @roberteg16 wrote:

Hi again @mizvekov, I've been spending a bit of time on resuming this issue.

When applying (after reverting the changes) the first change related to the Template case of profiling the as-written one name this resulted in the following test failing:
....
I've been looking around trying to figure out what's the issue but I am feeling that I lack the required knowledge to diagnose properly the problem.

I think we may be failing to canonicalize template arguments somewhere else where it's required.

We could be for example producing two different types because we produced two different template specializations for the same canonical template arguments.

The most likely suspect is Sema::CheckTemplateArgumentList in SemaTemplate.cpp.
That function is supposed to receive the arguments as written as input, and then produce a 'resolved' template argument list in the 'Converted' output parameter.

Looking at the helper function CheckTemplateArgument, right at the bottom for the Template and TemplateExpansion cases, we are pushing the argument into the Converted list without canonicalizing it.
I think that is one problem.

Back at CheckTemplateArgumentList, there are also some cases where we are perfoming pack expansions where we are potentially pushing non-canonical template arguments into that list, this is also worth looking over.

One question I want to ask is: On StmtProfiler::VisitTemplateArgument it states on a comment:

// Mostly repetitive with TemplateArgument::Profile!

but as far as I could see it really does not mimic the behavior in some cases. Is this at purpose?

Thanks for your time.

That one looks mostly correct, except perhaps that we should be profiling the number of known expansions for the TemplateExpansion case, but that goes for both implementations.

I don't know if this was done on purpose but then became invalid later with other changes, the commits which introduces these things are pretty ancient, they date back to SVN era, and there is a lack of explanations in the history.

We can try pinging whoever was involved at the time. I can't find the phab handle of the author of that commit, but it seems @rjmccall might have been involved and he is still around.

[mizvekov]

Hello, @roberteg16, are you still interested in working on this patch?

I might need a fix for this myself, and also it happened recently that someone else attempted a fix for the same bug.

If you are not abandoning it, please let us know!

[roberteg16]

In D126172#3754221, @mizvekov wrote:

Hello, @roberteg16, are you still interested in working on this patch?

I might need a fix for this myself, and also it happened recently that someone else attempted a fix for the same bug.

If you are not abandoning it, please let us know!

Hey @mizvekov! Sorry for the late response.

Yes, I am still interested and I am working on this. I'll let you know ASAP if I find something that resembles a valid fix.

I would be very grateful if you could give me some tips for debuging better clang or if you could point me towards documents, links, whatever that could give me nice information about how to reason about the code of the parser/sema of clang.

If you are in a hurry and need to fix it yourself and it happens that you finish the path first, please @ me so I can get to see how an error like this would be fixed, thanks!

[mizvekov]

In D126172#3767188, @roberteg16 wrote:

Hey @mizvekov! Sorry for the late response.

Yes, I am still interested and I am working on this. I'll let you know ASAP if I find something that resembles a valid fix.

I would be very grateful if you could give me some tips for debuging better clang or if you could point me towards documents, links, whatever that could give me nice information about how to reason about the code of the parser/sema of clang.

If you are in a hurry and need to fix it yourself and it happens that you finish the path first, please @ me so I can get to see how an error like this would be fixed, thanks!

Hello! Yeah you were just a bit too late, I have a patch that is accepted and should go in soon: https://reviews.llvm.org/D133072
I had run into this problem while making changes around CheckTemplateArgumentList, and I have this big patch which I am trying to offload into smaller chunks to expedite review.

I don't have any documents to recommend besides the obvious things, like the Clang docs auto-generated from the source code.
If you do want to get started, I can only recommend what worked for me, which is to try fixing bugs first, much like you tried here.
Though perhaps it's not very productive to get stuck in any one for a very long time, instead just ask for help or move on :)

I think that if you want to implement something new or a more substantial change, being able to debug quickly certainly helps.
There is no way you are going to be able to understand what is going on early on, so you will make a big mess, break hundreds of tests and so on, and being able to quickly go over and understand what is going on with them is important.
Invest in isolating bugs before working on them, it always pays off.

When fixing stuff, beware of changing assertions, most of the times the assertion is correct.
Also, if a bug fix is too local and seems easy, it's always worth spending some extra time to be sure.

And lots and lots and lots of free time, patience and being able to work without distractions.

clang/lib/AST/TemplateBase.cpp

bool TemplateArgument::structurallyEquals(const TemplateArgument &Other) const {
   case Null:
   case Type:
   case Expression:
   case NullPtr:
     return TypeOrValue.V == Other.TypeOrValue.V;
 
   case Template:
   case TemplateExpansion:
-    return TemplateArg.Name == Other.TemplateArg.Name &&
+    return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+               Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
            TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
     return getAsDecl() == Other.getAsDecl();
 
   case Integral:
     return getIntegralType() == Other.getIntegralType() &&
            getAsIntegral() == Other.getAsIntegral();
 

clang/test/CXX/dcl/dcl.fct/p17.cpp

namespace constrained {
   static_assert(is_same_v<decltype(S1('a')), S1>);
   // expected-error@-1{{no matching}}
   struct S2 { S2(C2<char> auto); };
   // expected-note@-1{{candidate template ignored}} expected-note@-1{{because}}
   // expected-note@-2 2{{candidate constructor}}
   static_assert(is_same_v<decltype(S2(1)), S2>);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v<decltype(S2('a')), S2>);
-}
+
+  // clang-format off
+  template <typename T> struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  void f23(C2<::S3> auto);
+  // expected-error@-1 {{no template named 'S3' in the global namespace; did you mean simply 'S3'?}}
+  // expected-error@-2 {{use of class template '::S3' requires template arguments}}
+  // clang-format on
+} // namespace constrained
+
+template <typename T> struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template arguments}}
+// clang-format on
+} // namespace constrained