[Clang][AST] Fix a crash on attaching doc comments

[chenshanzhi]

This crash is basically caused by calling
ASTContext::getRawCommentForDeclNoCacheImp with its input arguments RepresentativeLocForDecl and CommentsInTheFile refering to different files. A reduced reproducer is provided in this patch.

After the source locations for instantiations of funtion template are corrected in the commit 256a0b2, the variable CommitsInThisFile in the function
ASTContext::attachCommentsToJustParsedDecls would refer to the source file rather than the header file for implicit function template instantiation. Therefore, in the first loop in ASTContext::attachCommentsToJustParsedDecls, D should also be adjusted for relevant scenarios like the second loop.

Fixes #67979
Fixes #68524
Fixes #70550

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Shanzhi (chenshanzhi)

Changes

This crash is basically caused by calling
ASTContext::getRawCommentForDeclNoCacheImp with its input arguments RepresentativeLocForDecl and CommentsInTheFile refering to different files. A reduced reproducer is provided in this patch.

After the source locations for instantiations of funtion template are corrected in the commit 256a0b2, the variable CommitsInThisFile in the function
ASTContext::attachCommentsToJustParsedDecls would refer to the source file rather than the header file for implicit function template instantiation. Therefore, in the first loop in ASTContext::attachCommentsToJustParsedDecls, D should also be adjusted for relevant scenarios like the second loop.

Fixes #67979 #68524 #70550

---

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

2 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+5-1)
• (added) clang/test/AST/ast-crash-doc-function-template.cpp (+30)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 0fc0831b221aab..3abc526efd7de6 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -498,7 +498,11 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
     return;
 
   FileID File;
-  for (Decl *D : Decls) {
+  for (const Decl *D : Decls) {
+    if (D->isInvalidDecl())
+      continue;
+
+    D = &adjustDeclToTemplate(*D);
     SourceLocation Loc = D->getLocation();
     if (Loc.isValid()) {
       // See if there are any new comments that are not attached to a decl.
diff --git a/clang/test/AST/ast-crash-doc-function-template.cpp b/clang/test/AST/ast-crash-doc-function-template.cpp
new file mode 100644
index 00000000000000..d48eb0dbe02f01
--- /dev/null
+++ b/clang/test/AST/ast-crash-doc-function-template.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x c++ -Wdocumentation -fsyntax-only -ast-dump-all %t/t.cpp
+
+//--- t.h
+/// MyClass in the header file
+class MyClass {
+public:
+  template <typename T>
+  void Foo() const;
+
+  /// Bar
+  void Bar() const;
+};
+
+//--- t.cpp
+#include "t.h"
+
+/// MyClass::Bar: Foo<int>() is implicitly instantiated and called here.
+void MyClass::Bar() const {
+  Foo<int>();
+}
+
+/// MyClass::Foo
+template <typename T>
+void MyClass::Foo() const {
+}
+
+// CHECK: TranslationUnitDecl

[gribozavr]

Thank you for the fix.

Here's what happens here.

This function receives the just-parsed decls, and its aim is to the comment in the same vicinity (I know this because I am the original author of the code).

This first loop over Decls identifies the file in which we will be looking for comments. Then the code retrieves the comments in that file (auto CommentsInThisFile = Comments.getCommentsInFile(File);). At this point we have really committed which file we are looking in.

Now, the next loop over Decls disturbs this decision this by adjusting the decl to the template. That decl could be in a different file.

The call const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr); retrieves source locations for the adjusted decl. The adjusted decl could be in a different file from the comments that we retrieved in CommentsInThisFile.

Next, we are passing the mismatched DeclLocs and CommentsInThisFile into getRawCommentForDeclNoCacheImpl, which crashes when they come from different files.

I think the bug was introduced as a consequence of f31d8df and b67d370, while the recent source location change merely unmasked it.

So, this makes me wondering if there is a better way to structure this code to make the mismatch between DeclLocs and CommentsInThisFile impossible. I think a better way would be to merge the two loops over Decls and call Comments.getCommentsInFile for the source location from DeclLocs.

WDYT @chenshanzhi ? We can merge this change as is, since it is a good crash fix, but it would be great if you could make the code more robust by merging the loops.

[chenshanzhi]

@gribozavr
Thanks for your reply! I'm really glad that I can get the explanation about the original aim of the relevant code from you!

I do have considered merging the two loops as you recommended when I tried to solve the crash. But I got confused when I compared the implementation of ASTContext::attachCommentsToJustParsedDecls and ASTContext::getRawCommentForDeclNoCache. And I'm not sure whether what we want for ASTContext::attachCommentsToJustParsedDecls is just like a ASTContext::getRawCommentForDeclNoCache with additional cache operations after calling getRawCommentForDeclNoCacheImpl?
Actually, when I tried to solve the crash and glanced over the refactor changes in f31d8df, I thought the aim of introducing attachCommentsToJustParsedDecls might be achieving some level of speed-ups. And the comment // The location doesn't have to be precise - we care only about the file. made me think some of the speed-ups might come from the early break in the first loop and the author might be inclined to use two loops and an early break in the first loop. That's why I did not choose to merge the two loops in this crash fix commit. I totally agree that merging the two loops would make this code more robust. I think we could merge this crash fix change first and open another issue to track further improvements.

[itf]

@gribozavr, could we merge this fix? As a first-time contributor I think @chenshanzhi cannot merge it