[clang] AST Visitor: skip empty qualifiers in QualifiedTemplateName

[mizvekov]

This change was missed in #93433.

This fixes a problem detected by UBSAN build bots.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

This change was missed in #93433.

---

Full diff: https://github.com/llvm/llvm-project/pull/93926.diff

1 Files Affected:

• (modified) clang/include/clang/AST/RecursiveASTVisitor.h (+6-3)

diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index 4bbb4380cdd7f..28a90dffcb8dc 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -855,10 +855,13 @@ bool RecursiveASTVisitor<Derived>::TraverseDeclarationNameInfo(
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseTemplateName(TemplateName Template) {
-  if (DependentTemplateName *DTN = Template.getAsDependentTemplateName())
+  if (DependentTemplateName *DTN = Template.getAsDependentTemplateName()) {
     TRY_TO(TraverseNestedNameSpecifier(DTN->getQualifier()));
-  else if (QualifiedTemplateName *QTN = Template.getAsQualifiedTemplateName())
-    TRY_TO(TraverseNestedNameSpecifier(QTN->getQualifier()));
+  } else if (QualifiedTemplateName *QTN = Template.getAsQualifiedTemplateName()) {
+    if (T->getQualifier()) {
+      TRY_TO(TraverseNestedNameSpecifier(QTN->getQualifier()));
+    }
+  }
 
   return true;
 }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cor3ntin]

LGTM (modulo suggestion)

[cor3ntin]

Suggested change

	} else if (QualifiedTemplateName *QTN =
	Template.getAsQualifiedTemplateName()) {
	if (QTN->getQualifier()) {
	TRY_TO(TraverseNestedNameSpecifier(QTN->getQualifier()));
	}
	}
	} else if (QualifiedTemplateName *QTN =
	Template.getAsQualifiedTemplateName(); QTN && QTN->getQualifier()) {
	TRY_TO(TraverseNestedNameSpecifier(QTN->getQualifier()));
	}

[mizvekov]

I understand that looks cleaner at first glance, but the intention in the original code was for each arm to match each kind of TemplateName.

For example, this doesn't traverse OverloadTemplateName right now, but if we wanted to add it after this change, this would necessitate to refactor the if branches once again.

[mizvekov]

I am going to merge since this fixes broken build bot.

Let's continue in post.

[Sterling-Augustine]

Even with this fix, I'm seeing a segfault in clang/lib/AST/QualTypeNames.cpp at line 216.

  switch(Scope->getKind()) {

Scope is nullptr. The call is from line 70:

  QualifiedTemplateName *QTName = TName.getAsQualifiedTemplateName();

  if (QTName && !QTName->hasTemplateKeyword()) {
    NNS = QTName->getQualifier();
    NestedNameSpecifier *QNNS = getFullyQualifiedNestedNameSpecifier(
        Ctx, NNS, WithGlobalNsPrefix);

which means that the QTName->getQualifier() is returning nullptr where it didn't before. Checking QTName->getQualifier() for nullptr in the conditional (so it can take the else side), everything works. But it seems to me that there is probably a better fix that someone familiar with the code would be able to figure out. I'm not even sure the original is doing the right thing.

I have a fix in testing that adds this check, but if you would double check it after it goes in, that would be helpful.

Unfortunately the failure is in an internal tool, against internal sources, which makes getting a reproducer a long process. I'm also working on that.

[mizvekov]

Even with this fix, I'm seeing a segfault in clang/lib/AST/QualTypeNames.cpp at line 216.
I have a fix in testing that adds this check, but if you would double check it after it goes in, that would be helpful.

Unfortunately the failure is in an internal tool, against internal sources, which makes getting a reproducer a long process. I'm also working on that.

The fix gets the intended behavior right.

There are unittests for QualTypeNames, a new test case could go in there.