[clang-tidy][Sema][AST] Different behavior Sema and AST matcher

Clang Frontend
1

Hi!

As proposed by @whisperity I’m posting here to ask about a strange situation we found in the following bug fix:

https://reviews.llvm.org/D110614

We had a matcher to detect if a class has a public virtual destructor:

has(cxxDestructorDecl(isPublic(), isVirtual())),

However this didn’t work for class templates, see details:

https://reviews.llvm.org/D110614#3032760

The workaround was to create a custom matcher that seems to do the same thing as the original matcher, but it actually works for templates because it calls cxxDestructorDecl::isVirtual() (from Sema), which correctly propagates the virtual specifier from the base class.

Is this a concern, and should someone look into Sema/AST? (I’m unfortunately a n00b here). Or should we just merge the clang-tidy fix and move on?

Thanks!

2

Hi!

As proposed by @whisperity I’m posting here to ask about a strange situation we found in the following bug fix:

https://reviews.llvm.org/D110614

We had a matcher to detect if a class has a public virtual destructor:

has(cxxDestructorDecl(isPublic(), isVirtual())),

However this didn’t work for class templates, see details:

https://reviews.llvm.org/D110614#3032760

The workaround was to create a custom matcher that seems to do the same thing as the original matcher, but it actually works for templates because it calls cxxDestructorDecl::isVirtual() (from Sema), which correctly propagates the virtual specifier from the base class.

What does the original (buggy) implementation use instead of cxxDestructorDecl::isVirtual? If the original’s just using the wrong query for testing virtuality that seems like a reasonable thing to fix as a bug?

3

What does the original (buggy) implementation use instead of cxxDestructorDecl::isVirtual?

It used this matcher:

cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
unless(anyOf(
has(cxxDestructorDecl(isPublic(), isVirtual())),
has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
.bind(“ProblematicClassOrStruct”),

So the line:

has(cxxDestructorDecl(isPublic(), isVirtual())),

Should have done the job, but it didn’t.

4

What does the original (buggy) implementation use instead of cxxDestructorDecl::isVirtual?

It used this matcher:

cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
unless(anyOf(
has(cxxDestructorDecl(isPublic(), isVirtual())),
has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
.bind(“ProblematicClassOrStruct”),

So the line:

has(cxxDestructorDecl(isPublic(), isVirtual())),

Should have done the job, but it didn’t.

That’s the bit I’m trying to ask about/understand - do you know why “cxxDestructorDecl(isVirtual())” was inadequate, if “cxxDestructorDecl::isVirtual() (from Sema)” is adequate? I would’ve expected “cxxDestructorDecl(isVirtual())” to call CXXDestructorDecl::isVirtual - but I guess it doesn’t?

5

My reading is:

  • isVirtual() matcher indeed calls CXXMethodDecl::isVirtual
  • CXXMethodDecl::isVirtual does what you expect for non-template code: has “virtual” specifier or overrides anything
  • class template bodies don’t contain implicit CXXDestructorDecls. These are only created when the class is instantiated, probably because in the general case that’s when the analysis can happen. This is why the “unless” of your matcher does not apply.
  • I’m confused about how your patch fixes this, because my reading of the code is that getDestructor() on the class template body should return null. Will dig into this a little.
6

Thanks for the summary/jumping in here Sam - curious to hear what comes up, but glad it’s in your hands/someone with more context here!

7

Sorry, should have done more digging first.

The matcher does match the main template class body as suspicious, but then the check() callback immediately to look up a destructor and fails, so class template bodies will never be warned on directly. Rather the false positive happens when the template is instantiated.

If you look carefully at the AST dump, there are two declarations of the specialization TemplatedDerived: a ClassTemplateSpecializationDecl, and a CXXRecordDecl redeclaration nested within it: the injected-class-name. The latter is what is being matched.
The injected-class-name is a simple redeclaration. It has no children of its own, so it never matches has() which just refers to AST structure. However if you do semantic lookups on it, it will find things in the “main” declaration. It matches InheritsVirtualMethod, because hasAnyBase is a semantic lookup. It doesn’t match has(cxxDestructorDecl()) at all, but getDestructor() does work.
This is why your patch fixes that case, but it’s pretty indirect. It’d be better to exclude the injected-class-name directly I suppose. Maybe by excluding isImplicit()?

(Incidentally, I’m not sure why the derived class needs to be a template specialization for hasAnyBase() to match the injected-class-name, so still want to poke at this some more)

8

(Incidentally, I’m not sure why the derived class needs to be a template specialization for hasAnyBase() to match the injected-class-name, so still want to poke at this some more)

Hmm, the reason is that in a template instantiation, the injected-class-name is a redecl (“prev 0x…” in the AST dump) of the ClassTemplateSpecializationDecl.
Whereas for a “normal” class this is not the case. This affects the behavior of hasDefinition() and others.

Maybe there’s a good reason for this, but it seems very suspicious to me.

9

Looks like originally injected-class-names were considered redecls, then this was changed for CXXRecordDecl in 2010: https://github.com/llvm/llvm-project/commit/470c454a6176ef31474553e408c90f5ee630df89 but the corresponding place where templates were instantiated was not updated: https://github.com/llvm/llvm-project/blob/470c454a6176ef31474553e408c90f5ee630df89/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L930-L931

So I think this is an AST bug in the end, after all. I’ll see if a patch breaks anything…

10

Sent https://reviews.llvm.org/D112765.

11

Wow, amazing job figuring this out thanks a lot! I wonder if that will fix other problems related to templates, for example this one:
https://bugs.llvm.org/show_bug.cgi?id=51802

(The template in the constructor is irrelevant)

Can’t wait to try it out :slight_smile: