llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Yuxuan Shui (yshui)
<details>
<summary>Changes</summary>
Unsure if this is the correct fix. But let me explain what I know so far about
this problem:
1. Include cleaner goes over macro refs, and calculates their source locations:
https://github.com/llvm/llvm-project/blob/676efd0ffb717215c752f200fe14163732290dcc/clang-tools-extra/clangd/IncludeCleaner.cpp#L303-L305
2. It assumes the macro ref is in the main file. I think the intention is that
`ParsedAST::Macros` only contains macro usages in the main file.
3. `ParsedAST::Macros` is filled in by `CollectMainFileMacros`:
https://github.com/llvm/llvm-project/blob/676efd0ffb717215c752f200fe14163732290dcc/clang-tools-extra/clangd/CollectMacros.cpp#L27-L43
4. `CollectMainFileMacros` checks if the macro usage is in main file with `if
(InMainFile)`, and `InMainFile` is updated in
`CollectMainFileMacros::FileChange`.
5. I think the intention of `PPCallbacks::FileChange` is that it is called
whenever the lexer moves to a new file.
Above is what I am sure about, after that I am not quite sure what's happening.
I think macro expansions (`HandleIdentifier -> HandleMacroExpansion`) are
done out of the normal file change logic? Because I observed
`CollectMainFileMacros::add` being calling with source locations that are
clearly not in the same file as reported by the most recent `FileChange`.
The consequence of that is that `CollectMainFileMacros` will get offsets
related to one file, and those offsets will later be combined with another file
(the main file) into a source location in the include cleaner. Those source
locations will then be used to get a spelling, which triggers a assertion
failure at:
https://github.com/llvm/llvm-project/blob/497ea1d84951626dea5bf644fef2d99e145e21ac/clang/lib/Tooling/Syntax/Tokens.cpp#L382
* * *
What I did here is instead of using `InMainFile`/`PPCallbacks::FileChange` to
make sure the macro is in the main file, I check the `FileID` directly
---
Full diff: https://github.com/llvm/llvm-project/pull/99514.diff
1 Files Affected:
- (modified) clang-tools-extra/clangd/CollectMacros.cpp (+7-7)
``````````diff
diff --git a/clang-tools-extra/clangd/CollectMacros.cpp
b/clang-tools-extra/clangd/CollectMacros.cpp
index c5ba8d903ba48..bda5616447193 100644
--- a/clang-tools-extra/clangd/CollectMacros.cpp
+++ b/clang-tools-extra/clangd/CollectMacros.cpp
@@ -26,11 +26,12 @@ Range MacroOccurrence::toRange(const SourceManager &SM)
const {
void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI,
bool IsDefinition, bool InIfCondition) {
- if (!InMainFile)
- return;
auto Loc = MacroNameTok.getLocation();
if (Loc.isInvalid() || Loc.isMacroID())
return;
+ auto FID = SM.getFileID(Loc);
+ if (FID != SM.getMainFileID())
+ return;
auto Name = MacroNameTok.getIdentifierInfo()->getName();
Out.Names.insert(Name);
@@ -42,10 +43,8 @@ void CollectMainFileMacros::add(const Token &MacroNameTok,
const MacroInfo *MI,
Out.UnknownMacros.push_back({Start, End, IsDefinition, InIfCondition});
}
-void CollectMainFileMacros::FileChanged(SourceLocation Loc, FileChangeReason,
- SrcMgr::CharacteristicKind, FileID) {
- InMainFile = isInsideMainFile(Loc, SM);
-}
+void CollectMainFileMacros::FileChanged(SourceLocation, FileChangeReason,
+ SrcMgr::CharacteristicKind, FileID) {}
void CollectMainFileMacros::MacroExpands(const Token &MacroName,
const MacroDefinition &MD,
@@ -93,7 +92,8 @@ void CollectMainFileMacros::Defined(const Token &MacroName,
void CollectMainFileMacros::SourceRangeSkipped(SourceRange R,
SourceLocation EndifLoc) {
- if (!InMainFile)
+ auto FID = SM.getFileID(R.getBegin());
+ if (FID != SM.getMainFileID())
return;
Position Begin = sourceLocToPosition(SM, R.getBegin());
Position End = sourceLocToPosition(SM, R.getEnd());
``````````
</details>
https://github.com/llvm/llvm-project/pull/99514
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
