[clang-tools-extra] [clangd] fix crash in include cleaner (PR #99514)

[via cfe-commits]

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