[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
https://github.com/bc-lee closed https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
dhoepfl wrote:
> Since **I'm not an expert in clang AST**, it is hard to reduce the failing
> cases.
Double so for me.
> So the crash is happened because the result of `DeclLocDecomp.second -
> CommentEndOffset` is overflowed, so operations over `StringRef Text` is
> making the crash.
>
> **The best way to fix this issue** is to find out why they are not from the
> same source code and fix it. However, I'm not sure how to fix it, so I've
> made a patch to avoid the crash.
While your fix resolves the specific crash we noticed, it does not fix the
underlying problem. I found that the source of the problem is noted in [this
comment](https://github.com/llvm/llvm-project/blob/80737d2ddf05507d96cdd723fb33a6e44ac72a48/clang/lib/Sema/SemaDecl.cpp#L14892).
I tried to “fix” this by removing a few lines starting
[here](https://github.com/llvm/llvm-project/blob/80737d2ddf05507d96cdd723fb33a6e44ac72a48/clang/lib/AST/ASTContext.cpp#L575)
(maybe you could also remove lines 561-574?) and moving them a few lines down
to get:
```
void ASTContext::attachCommentsToJustParsedDecls(ArrayRef Decls,
const Preprocessor *PP) {
if (Comments.empty() || Decls.empty())
return;
FileID File;
for (Decl *D : Decls) {
SourceLocation Loc = D->getLocation();
if (Loc.isValid()) {
// See if there are any new comments that are not attached to a decl.
// The location doesn't have to be precise - we care only about the file.
File = SourceMgr.getDecomposedLoc(Loc).first;
break;
}
}
if (File.isInvalid())
return;
/* REMOVED HERE */
// There is at least one comment not attached to a decl.
// Maybe it should be attached to one of Decls?
//
// Note that this way we pick up not only comments that precede the
// declaration, but also comments that *follow* the declaration -- thanks to
// the lookahead in the lexer: we've consumed the semicolon and looked
// ahead through comments.
for (const Decl *D : Decls) {
assert(D);
if (D->isInvalidDecl())
continue;
D = (*D);
const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
continue;
if (DeclRawComments.count(D) > 0)
continue;
/* ADDED THIS */
FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
if (File.isInvalid())
continue;
auto CommentsInThisFile = Comments.getCommentsInFile(File);
if (!CommentsInThisFile || CommentsInThisFile->empty() ||
CommentsInThisFile->rbegin()->second->isAttached())
continue;
/* UNTIL HERE */
if (RawComment *const DocComment =
getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile)) {
cacheRawCommentForDecl(*D, *DocComment);
comments::FullComment *FC = DocComment->parse(*this, PP, D);
ParsedComments[D->getCanonicalDecl()] = FC;
}
}
}
```
That way it no longer crashes … I think this loads the correct comments for
each decl but I am not sure if this is the correct way to fix it (or even if it
still does what it is supposed to do).
https://github.com/llvm/llvm-project/pull/68525
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
bc-lee wrote: Since **I'm not an expert in clang AST**, it is hard to reduce the failing cases. According to my analysis, this crash only happens when the multiple files are involved, so code reduction tools like creduce doesn't helpful a lot. Instead, I'm providing an explanation of the crash with screenshots in my local environment. In my local environment, I was building Apple's LLVM with [dac71d2e8c4cdc9e0a1254dbf3716252c302d6a5](https://github.com/apple/llvm-project/tree/dac71d2e8c4cdc9e0a1254dbf3716252c302d6a5) commit. A single line containing `#include "clang/AST/ASTContext.h"` and `-Wdocumentation` flag is enough to reproduce the crash. (Note that I'm not making changes against Apple's LLVM. I'm just building Apple's LLVM(and Swift compiler) using the original LLVM ToT commit.) To explain the crash, I've made modifications to `clang/lib/AST/ASTContext.cpp`, as shown in the screenshot.  It seems that `ASTContext::getRawCommentForDeclNoCacheImpl`, `OffsetCommentBehindDecl`, which is from `CommentsInTheFile` is directing `clang/include/clang/AST/ASTContext.h` file. More precisely, `CommentBeforeDeclRawText` is `/// The type for the C sigjmp_buf type.` and `OffsetCommentBehindDeclRawText` is `/// The type for the C ucontext_t type.` and in this case. The offset of each element are `14832` and `14913`, respectively. However, `Buffer` which is given by `DeclLocDecomp.first` directs the another source code, `clang/include/clang/AST/ExternalASTSource.h`. Since `CommentEndOffset` is based on `CommentBeforeDecl`, it doesn't make sense to compare `DeclLocDecomp.second` and `CommentEndOffset`, as they are not from the same source code. So the crash is happened because the result of `DeclLocDecomp.second - CommentEndOffset` is overflowed, so operations over `StringRef Text` is making the crash. **The best way to fix this issue** is to find out why they are not from the same source code and fix it. However, I'm not sure how to fix it, so I've made a patch to avoid the crash. This logic is behind by `CommentBeforeDecl->isDocumentation()`, and the crash occurs only when the `-Wdocumentation` flag is enabled. I believe that this logic is intended for aggregating comments to explain the reason for the `-Wdocumentation` warning. In other words, clang crashes when it attempts to provide an explanation for the warning. Therefore, **it might be acceptable to bypass this logic instead of crashing.** https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
AaronBallman wrote: > > Is there a way we could come up with a test for this? > > Unfortunately, I don't think so. I cannot reduce the 600k lines of > preprocessed code to a small test case that will crash the clang frontend. Have you tried using creduce or other such tool? (We generally don't accept patches [without test coverage](https://llvm.org/docs/DeveloperPolicy.html#test-cases) unless it really isn't possible to test the changes, that doesn't appear to be the situation here though.) https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
bc-lee wrote: > Is there a way we could come up with a test for this? Unfortunately, I don't think so. I cannot reduce the 600k lines of preprocessed code to a small test case that will crash the clang frontend. https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
cor3ntin wrote: Is there a way we could come up with a test for this? https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
https://github.com/bc-lee updated https://github.com/llvm/llvm-project/pull/68525 >From b272173acb8c2f92b5c5b7ebca4c00e4ac21037c Mon Sep 17 00:00:00 2001 From: Byoungchan Lee Date: Sun, 8 Oct 2023 21:47:05 +0900 Subject: [PATCH] [Clang][Frontend] Fix a crash when -Wdocumentation is used This commit resolves a crash issue in Clang's frontend caused while using the `-Wdocumentation` compiler flag. The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the `ASTContext.cpp` file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset. This could lead to an invalid length being passed to the `StringRef` constructor, triggering the crash. I have added a check to prevent this crash from occurring. Fixes #68524. --- clang/lib/AST/ASTContext.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index cdc3d62bca00873..7b4a4202921281c 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -344,6 +344,9 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl( if (Invalid) return nullptr; + if (DeclLocDecomp.second < CommentEndOffset) +return nullptr; + // Extract text between the comment and declaration. StringRef Text(Buffer + CommentEndOffset, DeclLocDecomp.second - CommentEndOffset); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
llvmbot wrote: @llvm/pr-subscribers-clang Changes This commit resolves a crash issue in Clang's frontend caused while using the `-Wdocumentation` compiler flag. The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the `ASTContext.cpp` file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset. This could lead to an invalid length being passed to the `StringRef` constructor, triggering the crash. I have added a check to prevent this crash from occurring. Fixes #68524. --- Full diff: https://github.com/llvm/llvm-project/pull/68525.diff 1 Files Affected: - (modified) clang/lib/AST/ASTContext.cpp (+3) ``diff diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index cdc3d62bca00873..7b4a4202921281c 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -344,6 +344,9 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl( if (Invalid) return nullptr; + if (DeclLocDecomp.second < CommentEndOffset) +return nullptr; + // Extract text between the comment and declaration. StringRef Text(Buffer + CommentEndOffset, DeclLocDecomp.second - CommentEndOffset); `` https://github.com/llvm/llvm-project/pull/68525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Frontend] Fix a crash when -Wdocumentation is used (PR #68525)
https://github.com/bc-lee created https://github.com/llvm/llvm-project/pull/68525 This commit resolves a crash issue in Clang's frontend caused while using the `-Wdocumentation` compiler flag. The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the `ASTContext.cpp` file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset. This could lead to an invalid length being passed to the `StringRef` constructor, triggering the crash. I have added a check to prevent this crash from occurring. Fixes #68524. >From 83977fda4860a6b2a99c9f5ad166fd62a8735da1 Mon Sep 17 00:00:00 2001 From: Byoungchan Lee Date: Sun, 8 Oct 2023 21:47:05 +0900 Subject: [PATCH] [Clang][Frontend] Fix a crash when -Wdocumentation is used This commit resolves a crash issue in Clang's frontend caused while using the `-Wdocumentation` compiler flag. The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the `ASTContext.cpp` file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset. This could lead to an invalid length being passed to the `StringRef` constructor, triggering the crash. I have added a check to prevent this crash from occurring. Fixes #68524. --- clang/lib/AST/ASTContext.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index cdc3d62bca00873..7b4a4202921281c 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -344,6 +344,9 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl( if (Invalid) return nullptr; + if (DeclLocDecomp.second < CommentEndOffset) +return nullptr; + // Extract text between the comment and declaration. StringRef Text(Buffer + CommentEndOffset, DeclLocDecomp.second - CommentEndOffset); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
