[llvm] [clang-tools-extra] [clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
https://github.com/vfdff closed https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
itf wrote: @gribozavr, could we merge this fix? As a first-time contributor I think @chenshanzhi cannot merge it https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
https://github.com/chenshanzhi updated
https://github.com/llvm/llvm-project/pull/78716
>From dcdf4a5825a402ab1392f61354c604a9cf965bdd Mon Sep 17 00:00:00 2001
From: Shanzhi Chen
Date: Thu, 18 Jan 2024 11:40:09 +
Subject: [PATCH] [Clang][AST] Fix a crash on attaching doc comments
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 256a0b298c68b89688b80350b034daf2f7785b67, 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
---
clang/lib/AST/ASTContext.cpp | 6 +++-
.../AST/ast-crash-doc-function-template.cpp | 30 +++
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 clang/test/AST/ast-crash-doc-function-template.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 5eb7aa3664569d..9a0ede2010596e 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -498,7 +498,11 @@ void
ASTContext::attachCommentsToJustParsedDecls(ArrayRef Decls,
return;
FileID File;
- for (Decl *D : Decls) {
+ for (const Decl *D : Decls) {
+if (D->isInvalidDecl())
+ continue;
+
+D = (*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 00..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
+ void Foo() const;
+
+ /// Bar
+ void Bar() const;
+};
+
+//--- t.cpp
+#include "t.h"
+
+/// MyClass::Bar: Foo() is implicitly instantiated and called here.
+void MyClass::Bar() const {
+ Foo();
+}
+
+/// MyClass::Foo
+template
+void MyClass::Foo() const {
+}
+
+// CHECK: TranslationUnitDecl
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
chenshanzhi wrote: @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 f31d8df1c8c69e7a787c1c1c529a524f3001c66a, 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 inlined 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. https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
https://github.com/gribozavr approved this pull request. https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
gribozavr wrote: 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 f31d8df1c8c69 and b67d3702577d4, 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. https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
https://github.com/chenshanzhi edited https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
llvmbot wrote:
@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 256a0b298c68b89688b80350b034daf2f7785b67, 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
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 Decls,
return;
FileID File;
- for (Decl *D : Decls) {
+ for (const Decl *D : Decls) {
+if (D->isInvalidDecl())
+ continue;
+
+D = (*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 00..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
+ void Foo() const;
+
+ /// Bar
+ void Bar() const;
+};
+
+//--- t.cpp
+#include "t.h"
+
+/// MyClass::Bar: Foo() is implicitly instantiated and called here.
+void MyClass::Bar() const {
+ Foo();
+}
+
+/// MyClass::Foo
+template
+void MyClass::Foo() const {
+}
+
+// CHECK: TranslationUnitDecl
``
https://github.com/llvm/llvm-project/pull/78716
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
github-actions[bot] wrote: 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](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/78716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AST] Fix a crash on attaching doc comments (PR #78716)
https://github.com/chenshanzhi created
https://github.com/llvm/llvm-project/pull/78716
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 256a0b298c68b89688b80350b034daf2f7785b67, 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
>From c89482513c5b3e1f8f9495c8b6f04a5fd285404a Mon Sep 17 00:00:00 2001
From: Shanzhi Chen
Date: Thu, 18 Jan 2024 11:40:09 +
Subject: [PATCH] [Clang][AST] Fix a crash on attaching doc comments
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 256a0b298c68b89688b80350b034daf2f7785b67, 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
---
clang/lib/AST/ASTContext.cpp | 6 +++-
.../AST/ast-crash-doc-function-template.cpp | 30 +++
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 clang/test/AST/ast-crash-doc-function-template.cpp
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 Decls,
return;
FileID File;
- for (Decl *D : Decls) {
+ for (const Decl *D : Decls) {
+if (D->isInvalidDecl())
+ continue;
+
+D = (*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 00..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
+ void Foo() const;
+
+ /// Bar
+ void Bar() const;
+};
+
+//--- t.cpp
+#include "t.h"
+
+/// MyClass::Bar: Foo() is implicitly instantiated and called here.
+void MyClass::Bar() const {
+ Foo();
+}
+
+/// MyClass::Foo
+template
+void MyClass::Foo() const {
+}
+
+// CHECK: TranslationUnitDecl
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
