[PATCH] D144054: [Lex] Fix a crash in updateConsecutiveMacroArgTokens.

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 22 00:50:01 PST 2023


sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1029
+      // Including Limit will not mischeck for across-file-id tokens
+      // because SourceManager allocates FileSize + 1 for each SLocEntry.
+      //
----------------
Naively, it seems like if recovery tokens could lead us to read one-past-the-end, it could also cause us to read two-past-the-end in which case this check would fail.

We're relying on this never happening, so I think the comment should refer more specifically to the tokens that get inserted.

Maybe "recovery only ever inserts a single token past the end of the FileID, specifically the `)` when a macro-arg containing a comma should be guarded by parentheses"?


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1031
+      //
+      // See https://github.com/llvm/llvm-project/issues/60722.
+      return T.getLocation() >= BeginLoc && T.getLocation() <= Limit
----------------
hokein wrote:
> shafik wrote:
> > I don't believe we include links to issues in comments but you should definitely add the information to the commit message and when folks look at the commit they can get the that context there.
> I'm happy to remove it. But is there any guideline discouraging this practise? I have already seen this pattern in LLVM project. I think this is based on the author's judgement (IMO, it seems better to keep it for this case).
I see 50+ refs to github issues, 70+ links to bugs.llvm.org, 1K+ references to PRNNNNN.
The reference seems useful when it's a strange workaround like this.


================
Comment at: clang/test/Lexer/update_consecutive_macro_crash.cpp:8
+void foo() {
+  X(int{,}); // expected-error {{too many arguments provided to function-like macro invocation}} \
+                 expected-error {{expected expression}} \
----------------
hokein wrote:
> More details about the issue :
> 
> - due to the error recovery, the clang lexer inserts a pair of `()` around the macro argument `int{,}`, so we will see [`(`, `int`, `{`, `,`, `}`, `)`] tokens
> - however, the size of file id for the macro argument only take account the written tokens which are [`int`, `{`, `,`, `}`], and the extra inserted `)` token is at the `Limit` source location which triggers an empty `Partition`. 
can you add these as comments to the test?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144054/new/

https://reviews.llvm.org/D144054



More information about the cfe-commits mailing list