Page MenuHomePhabricator

[AST] RAV doesn't traverse explicitly instantiated function bodies by default
Needs ReviewPublic

Authored by sammccall on Feb 24 2022, 10:34 AM.

Details

Reviewers
hokein
davidxl
Summary

This is consistent with the idea of shouldVisitTemplateInstantiations():
when false, we should traverse the code the user wrote but not the code
clang synthesized. Therefore we should be able to traverse the
signature of the function without traversing its body.
This is also consistent with how class and variable templates behave.

Note that currently RAV does not traverse *any* part of the these when
shouldVisitTemplateInstantiations() is false (which is another bug).
So currently this change is only observable when the user passes the
instantiated FunctionDecl to TraverseDecl themselves.

This is what the caller in CodeGenPGO does, and it wants to traverse the
instantiated body. I believe not traversing instantiations in general in
this context is an oversight.

Diff Detail

Event Timeline

sammccall created this revision.Feb 24 2022, 10:34 AM
sammccall requested review of this revision.Feb 24 2022, 10:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 24 2022, 10:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@davidxl does the one-line CodeGenPGO change look reasonable? If you're not the person to ask, could you recommend someone? Thanks!

(Applying the patch without the CodeGenPGO change causes Profile/cxx-templates.cpp and CoverageMapping/branch-templates.cpp to fail as when mapRegionCounters() gets an instantiated template function it skips traversing the body)

nridge added a subscriber: nridge.Feb 24 2022, 10:14 PM
hokein added inline comments.Feb 25 2022, 1:22 AM
clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
168

What does the Head mean?

209

There are many declaration in the testcode, hard to figure out which one we are chosen, can you add a comment about which declaration is chosen as the traversal root? I think it is function decl instantiated by the template int foo<4>();?

hokein added inline comments.Feb 25 2022, 1:25 AM
clang/include/clang/AST/RecursiveASTVisitor.h
2107

moving this part of code to if (VisitBody) seems like a different fix (and I think it is reasonable, from the grammar, the ctor-initializer is part of the function-body), can we separate it and add a test?

sammccall added inline comments.Feb 25 2022, 4:29 AM
clang/include/clang/AST/RecursiveASTVisitor.h
2107

I can do it as a separate patch before if you like, but it's NFC.

Behavior only differs if there is an init list and VisitBody is false.
However !VisitBody requires one of:

  • this is a declaration only => no init list
  • this is a defaulted function => no init list
clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
168

Within the function

int foo<4>() { return 0; }.

I've called int foo<4>() the "head" and {return 0;} the "body".
The distinction is that the head was written, and the body was instantiated.

are there better terms? (I'd probably like this "head" better than "declaration part" so maybe we should just define these terms in the shouldVisitTemplateInstantiations() comment)

davidxl added inline comments.Feb 27 2022, 4:34 PM
clang/lib/CodeGen/CodeGenPGO.cpp
157

This one line change looks ok to me.

Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 10:51 PM