Skip to content

Commit 27e075f

Browse files
sam-mccalltru
sam-mccall
authored and
tru
committed
[Syntax] Fix macro-arg handling in TokenBuffer::spelledForExpanded
A few cases were not handled correctly. Notably: #define ID(X) X #define HIDE a ID(b) HIDE spelledForExpanded() would claim HIDE is an equivalent range of the 'b' it contains, despite the fact that HIDE also covers 'a'. While trying to fix this bug, I found findCommonRangeForMacroArgs hard to understand (both the implementation and how it's used in spelledForExpanded). It relies on details of the SourceLocation graph that are IMO fairly obscure. So I've added/revised quite a lot of comments and made some naming tweaks. Fixes clangd/clangd#1289 Differential Revision: https://reviews.llvm.org/D134618 (cherry picked from commit 67268ee)
1 parent 359ef0c commit 27e075f

File tree

2 files changed

+211
-63
lines changed

2 files changed

+211
-63
lines changed

‎clang/lib/Tooling/Syntax/Tokens.cpp

Lines changed: 155 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -55,45 +55,140 @@ getTokensCovering(llvm::ArrayRef<syntax::Token> Toks, SourceRange R,
5555
return {Begin, End};
5656
}
5757

58-
// Finds the smallest expansion range that contains expanded tokens First and
59-
// Last, e.g.:
58+
// Finds the range within FID corresponding to expanded tokens [First, Last].
59+
// Prev precedes First and Next follows Last, these must *not* be included.
60+
// If no range satisfies the criteria, returns an invalid range.
61+
//
6062
// #define ID(x) x
6163
// ID(ID(ID(a1) a2))
6264
// ~~ -> a1
6365
// ~~ -> a2
6466
// ~~~~~~~~~ -> a1 a2
65-
SourceRange findCommonRangeForMacroArgs(const syntax::Token &First,
66-
const syntax::Token &Last,
67-
const SourceManager &SM) {
68-
SourceRange Res;
69-
auto FirstLoc = First.location(), LastLoc = Last.location();
70-
// Keep traversing up the spelling chain as longs as tokens are part of the
71-
// same expansion.
72-
while (!FirstLoc.isFileID() && !LastLoc.isFileID()) {
73-
auto ExpInfoFirst = SM.getSLocEntry(SM.getFileID(FirstLoc)).getExpansion();
74-
auto ExpInfoLast = SM.getSLocEntry(SM.getFileID(LastLoc)).getExpansion();
75-
// Stop if expansions have diverged.
76-
if (ExpInfoFirst.getExpansionLocStart() !=
77-
ExpInfoLast.getExpansionLocStart())
67+
SourceRange spelledForExpandedSlow(SourceLocation First, SourceLocation Last,
68+
SourceLocation Prev, SourceLocation Next,
69+
FileID TargetFile,
70+
const SourceManager &SM) {
71+
// There are two main parts to this algorithm:
72+
// - identifying which spelled range covers the expanded tokens
73+
// - validating that this range doesn't cover any extra tokens (First/Last)
74+
//
75+
// We do these in order. However as we transform the expanded range into the
76+
// spelled one, we adjust First/Last so the validation remains simple.
77+
78+
assert(SM.getSLocEntry(TargetFile).isFile());
79+
// In most cases, to select First and Last we must return their expansion
80+
// range, i.e. the whole of any macros they are included in.
81+
//
82+
// When First and Last are part of the *same macro arg* of a macro written
83+
// in TargetFile, we that slice of the arg, i.e. their spelling range.
84+
//
85+
// Unwrap such macro calls. If the target file has A(B(C)), the
86+
// SourceLocation stack of a token inside C shows us the expansion of A first,
87+
// then B, then any macros inside C's body, then C itself.
88+
// (This is the reverse of the order the PP applies the expansions in).
89+
while (First.isMacroID() && Last.isMacroID()) {
90+
auto DecFirst = SM.getDecomposedLoc(First);
91+
auto DecLast = SM.getDecomposedLoc(Last);
92+
auto &ExpFirst = SM.getSLocEntry(DecFirst.first).getExpansion();
93+
auto &ExpLast = SM.getSLocEntry(DecLast.first).getExpansion();
94+
95+
if (!ExpFirst.isMacroArgExpansion() || !ExpLast.isMacroArgExpansion())
96+
break;
97+
// Locations are in the same macro arg if they expand to the same place.
98+
// (They may still have different FileIDs - an arg can have >1 chunks!)
99+
if (ExpFirst.getExpansionLocStart() != ExpLast.getExpansionLocStart())
78100
break;
79-
// Do not continue into macro bodies.
80-
if (!ExpInfoFirst.isMacroArgExpansion() ||
81-
!ExpInfoLast.isMacroArgExpansion())
101+
// Careful, given:
102+
// #define HIDE ID(ID(a))
103+
// ID(ID(HIDE))
104+
// The token `a` is wrapped in 4 arg-expansions, we only want to unwrap 2.
105+
// We distinguish them by whether the macro expands into the target file.
106+
// Fortunately, the target file ones will always appear first.
107+
auto &ExpMacro =
108+
SM.getSLocEntry(SM.getFileID(ExpFirst.getExpansionLocStart()))
109+
.getExpansion();
110+
if (ExpMacro.getExpansionLocStart().isMacroID())
82111
break;
83-
FirstLoc = SM.getImmediateSpellingLoc(FirstLoc);
84-
LastLoc = SM.getImmediateSpellingLoc(LastLoc);
85-
// Update the result afterwards, as we want the tokens that triggered the
86-
// expansion.
87-
Res = {FirstLoc, LastLoc};
112+
// Replace each endpoint with its spelling inside the macro arg.
113+
// (This is getImmediateSpellingLoc without repeating lookups).
114+
First = ExpFirst.getSpellingLoc().getLocWithOffset(DecFirst.second);
115+
Last = ExpLast.getSpellingLoc().getLocWithOffset(DecLast.second);
116+
117+
// Now: how do we adjust the previous/next bounds? Three cases:
118+
// A) If they are also part of the same macro arg, we translate them too.
119+
// This will ensure that we don't select any macros nested within the
120+
// macro arg that cover extra tokens. Critical case:
121+
// #define ID(X) X
122+
// ID(prev target) // selecting 'target' succeeds
123+
// #define LARGE ID(prev target)
124+
// LARGE // selecting 'target' fails.
125+
// B) They are not in the macro at all, then their expansion range is a
126+
// sibling to it, and we can safely substitute that.
127+
// #define PREV prev
128+
// #define ID(X) X
129+
// PREV ID(target) // selecting 'target' succeeds.
130+
// #define LARGE PREV ID(target)
131+
// LARGE // selecting 'target' fails.
132+
// C) They are in a different arg of this macro, or the macro body.
133+
// Now selecting the whole macro arg is fine, but the whole macro is not.
134+
// Model this by setting using the edge of the macro call as the bound.
135+
// #define ID2(X, Y) X Y
136+
// ID2(prev, target) // selecting 'target' succeeds
137+
// #define LARGE ID2(prev, target)
138+
// LARGE // selecting 'target' fails
139+
auto AdjustBound = [&](SourceLocation &Bound) {
140+
if (Bound.isInvalid() || !Bound.isMacroID()) // Non-macro must be case B.
141+
return;
142+
auto DecBound = SM.getDecomposedLoc(Bound);
143+
auto &ExpBound = SM.getSLocEntry(DecBound.first).getExpansion();
144+
if (ExpBound.isMacroArgExpansion() &&
145+
ExpBound.getExpansionLocStart() == ExpFirst.getExpansionLocStart()) {
146+
// Case A: translate to (spelling) loc within the macro arg.
147+
Bound = ExpBound.getSpellingLoc().getLocWithOffset(DecBound.second);
148+
return;
149+
}
150+
while (Bound.isMacroID()) {
151+
SourceRange Exp = SM.getImmediateExpansionRange(Bound).getAsRange();
152+
if (Exp.getBegin() == ExpMacro.getExpansionLocStart()) {
153+
// Case B: bounds become the macro call itself.
154+
Bound = (&Bound == &Prev) ? Exp.getBegin() : Exp.getEnd();
155+
return;
156+
}
157+
// Either case C, or expansion location will later find case B.
158+
// We choose the upper bound for Prev and the lower one for Next:
159+
// ID(prev) target ID(next)
160+
// ^ ^
161+
// new-prev new-next
162+
Bound = (&Bound == &Prev) ? Exp.getEnd() : Exp.getBegin();
163+
}
164+
};
165+
AdjustBound(Prev);
166+
AdjustBound(Next);
88167
}
89-
// Normally mapping back to expansion location here only changes FileID, as
90-
// we've already found some tokens expanded from the same macro argument, and
91-
// they should map to a consecutive subset of spelled tokens. Unfortunately
92-
// SourceManager::isBeforeInTranslationUnit discriminates sourcelocations
93-
// based on their FileID in addition to offsets. So even though we are
94-
// referring to same tokens, SourceManager might tell us that one is before
95-
// the other if they've got different FileIDs.
96-
return SM.getExpansionRange(CharSourceRange(Res, true)).getAsRange();
168+
169+
// In all remaining cases we need the full containing macros.
170+
// If this overlaps Prev or Next, then no range is possible.
171+
SourceRange Candidate =
172+
SM.getExpansionRange(SourceRange(First, Last)).getAsRange();
173+
auto DecFirst = SM.getDecomposedExpansionLoc(Candidate.getBegin());
174+
auto DecLast = SM.getDecomposedLoc(Candidate.getEnd());
175+
// Can end up in the wrong file due to bad input or token-pasting shenanigans.
176+
if (Candidate.isInvalid() || DecFirst.first != TargetFile || DecLast.first != TargetFile)
177+
return SourceRange();
178+
// Check bounds, which may still be inside macros.
179+
if (Prev.isValid()) {
180+
auto Dec = SM.getDecomposedLoc(SM.getExpansionRange(Prev).getBegin());
181+
if (Dec.first != DecFirst.first || Dec.second >= DecFirst.second)
182+
return SourceRange();
183+
}
184+
if (Next.isValid()) {
185+
auto Dec = SM.getDecomposedLoc(SM.getExpansionRange(Next).getEnd());
186+
if (Dec.first != DecLast.first || Dec.second <= DecLast.second)
187+
return SourceRange();
188+
}
189+
// Now we know that Candidate is a file range that covers [First, Last]
190+
// without encroaching on {Prev, Next}. Ship it!
191+
return Candidate;
97192
}
98193

99194
} // namespace
@@ -363,51 +458,48 @@ TokenBuffer::spelledForExpanded(llvm::ArrayRef<syntax::Token> Expanded) const {
363458
// of the range, bail out in that case.
364459
if (Expanded.empty())
365460
return llvm::None;
461+
const syntax::Token *First = &Expanded.front();
462+
const syntax::Token *Last = &Expanded.back();
463+
auto [FirstSpelled, FirstMapping] = spelledForExpandedToken(First);
464+
auto [LastSpelled, LastMapping] = spelledForExpandedToken(Last);
366465

367-
const syntax::Token *BeginSpelled;
368-
const Mapping *BeginMapping;
369-
std::tie(BeginSpelled, BeginMapping) =
370-
spelledForExpandedToken(&Expanded.front());
371-
372-
const syntax::Token *LastSpelled;
373-
const Mapping *LastMapping;
374-
std::tie(LastSpelled, LastMapping) =
375-
spelledForExpandedToken(&Expanded.back());
376-
377-
FileID FID = SourceMgr->getFileID(BeginSpelled->location());
466+
FileID FID = SourceMgr->getFileID(FirstSpelled->location());
378467
// FIXME: Handle multi-file changes by trying to map onto a common root.
379468
if (FID != SourceMgr->getFileID(LastSpelled->location()))
380469
return llvm::None;
381470

382471
const MarkedFile &File = Files.find(FID)->second;
383472

384-
// If both tokens are coming from a macro argument expansion, try and map to
385-
// smallest part of the macro argument. BeginMapping && LastMapping check is
386-
// only for performance, they are a prerequisite for Expanded.front() and
387-
// Expanded.back() being part of a macro arg expansion.
388-
if (BeginMapping && LastMapping &&
389-
SourceMgr->isMacroArgExpansion(Expanded.front().location()) &&
390-
SourceMgr->isMacroArgExpansion(Expanded.back().location())) {
391-
auto CommonRange = findCommonRangeForMacroArgs(Expanded.front(),
392-
Expanded.back(), *SourceMgr);
393-
// It might be the case that tokens are arguments of different macro calls,
394-
// in that case we should continue with the logic below instead of returning
395-
// an empty range.
396-
if (CommonRange.isValid())
397-
return getTokensCovering(File.SpelledTokens, CommonRange, *SourceMgr);
473+
// If the range is within one macro argument, the result may be only part of a
474+
// Mapping. We must use the general (SourceManager-based) algorithm.
475+
if (FirstMapping && FirstMapping == LastMapping &&
476+
SourceMgr->isMacroArgExpansion(First->location()) &&
477+
SourceMgr->isMacroArgExpansion(Last->location())) {
478+
// We use excluded Prev/Next token for bounds checking.
479+
SourceLocation Prev = (First == &ExpandedTokens.front())
480+
? SourceLocation()
481+
: (First - 1)->location();
482+
SourceLocation Next = (Last == &ExpandedTokens.back())
483+
? SourceLocation()
484+
: (Last + 1)->location();
485+
SourceRange Range = spelledForExpandedSlow(
486+
First->location(), Last->location(), Prev, Next, FID, *SourceMgr);
487+
if (Range.isInvalid())
488+
return llvm::None;
489+
return getTokensCovering(File.SpelledTokens, Range, *SourceMgr);
398490
}
399491

492+
// Otherwise, use the fast version based on Mappings.
400493
// Do not allow changes that doesn't cover full expansion.
401-
unsigned BeginExpanded = Expanded.begin() - ExpandedTokens.data();
402-
unsigned EndExpanded = Expanded.end() - ExpandedTokens.data();
403-
if (BeginMapping && BeginExpanded != BeginMapping->BeginExpanded)
494+
unsigned FirstExpanded = Expanded.begin() - ExpandedTokens.data();
495+
unsigned LastExpanded = Expanded.end() - ExpandedTokens.data();
496+
if (FirstMapping && FirstExpanded != FirstMapping->BeginExpanded)
404497
return llvm::None;
405-
if (LastMapping && LastMapping->EndExpanded != EndExpanded)
498+
if (LastMapping && LastMapping->EndExpanded != LastExpanded)
406499
return llvm::None;
407-
// All is good, return the result.
408500
return llvm::makeArrayRef(
409-
BeginMapping ? File.SpelledTokens.data() + BeginMapping->BeginSpelled
410-
: BeginSpelled,
501+
FirstMapping ? File.SpelledTokens.data() + FirstMapping->BeginSpelled
502+
: FirstSpelled,
411503
LastMapping ? File.SpelledTokens.data() + LastMapping->EndSpelled
412504
: LastSpelled + 1);
413505
}

‎clang/unittests/Tooling/Syntax/TokensTest.cpp

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,62 @@ TEST_F(TokenBufferTest, SpelledByExpanded) {
743743
ValueIs(SameRange(findSpelled("ID2 ( a4 , a5 a6 a7 )"))));
744744
// Should fail, spans multiple invocations.
745745
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("a1 a2 a3 a4")), llvm::None);
746+
747+
// https://github.com/clangd/clangd/issues/1289
748+
recordTokens(R"cpp(
749+
#define FOO(X) foo(X)
750+
#define INDIRECT FOO(y)
751+
INDIRECT // expands to foo(y)
752+
)cpp");
753+
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("y")), llvm::None);
754+
755+
recordTokens(R"cpp(
756+
#define FOO(X) a X b
757+
FOO(y)
758+
)cpp");
759+
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("y")),
760+
ValueIs(SameRange(findSpelled("y"))));
761+
762+
recordTokens(R"cpp(
763+
#define ID(X) X
764+
#define BAR ID(1)
765+
BAR
766+
)cpp");
767+
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("1")),
768+
ValueIs(SameRange(findSpelled(") BAR").drop_front())));
769+
770+
// Critical cases for mapping of Prev/Next in spelledForExpandedSlow.
771+
recordTokens(R"cpp(
772+
#define ID(X) X
773+
ID(prev ID(good))
774+
#define LARGE ID(prev ID(bad))
775+
LARGE
776+
)cpp");
777+
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
778+
ValueIs(SameRange(findSpelled("good"))));
779+
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), llvm::None);
780+
781+
recordTokens(R"cpp(
782+
#define PREV prev
783+
#define ID(X) X
784+
PREV ID(good)
785+
#define LARGE PREV ID(bad)
786+
LARGE
787+
)cpp");
788+
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
789+
ValueIs(SameRange(findSpelled("good"))));
790+
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), llvm::None);
791+
792+
recordTokens(R"cpp(
793+
#define ID(X) X
794+
#define ID2(X, Y) X Y
795+
ID2(prev, ID(good))
796+
#define LARGE ID2(prev, bad)
797+
LARGE
798+
)cpp");
799+
EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
800+
ValueIs(SameRange(findSpelled("good"))));
801+
EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), llvm::None);
746802
}
747803

748804
TEST_F(TokenBufferTest, ExpandedTokensForRange) {

0 commit comments

Comments
 (0)