[Lex] Fix a crash in updateConsecutiveMacroArgTokens.

[hokein]

Summary

Fixes https://github.com/llvm/llvm-project/issues/60722.

[hokein]

clang/test/Lexer/update_consecutive_macro_crash.cpp

9

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.

[shafik]

clang/lib/Lex/TokenLexer.cpp

1022–1031

You had refactored the previous version, was this the way it was handled before? Do we need to add more test to ensure that we don't have any other unintended issues?

1030

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.

[hokein]

clang/lib/Lex/TokenLexer.cpp

1022–1031

was this the way it was handled before?

Yeah, the previous implementation literally checked whether every token is from the same file id by calling getFileID.

Do we need to add more test to ensure that we don't have any other unintended issues?

I'd like to add more, but it is hard to foresee all invalid cases (clang has different error-recovery strategies that might have different side effects).

1030

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).

[sammccall]

clang/lib/Lex/TokenLexer.cpp

1028

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"?

1030

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.

clang/test/Lexer/update_consecutive_macro_crash.cpp

9

can you add these as comments to the test?

[hokein]

address comments.

clang/lib/Lex/TokenLexer.cpp

if (BeginLoc.isFileID()) {
     });
   } else {
     // Call getFileID once to calculate the bounds, and use the cheaper
     // sourcelocation-against-bounds comparison.
     FileID BeginFID = SM.getFileID(BeginLoc);
     SourceLocation Limit =
         SM.getComposedLoc(BeginFID, SM.getFileIDSize(BeginFID));
     Partition = All.take_while([&](const Token &T) {
-      return T.getLocation() >= BeginLoc && T.getLocation() < Limit &&
-             NearLast(T.getLocation());
+      // NOTE: the Limit is included! The lexer 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.
+      //
+      // It is safe to include the Limit here because SourceManager allocates
+      // FileSize + 1 for each SLocEntry.
+      //
+      // See https://github.com/llvm/llvm-project/issues/60722.
+      return T.getLocation() >= BeginLoc && T.getLocation() <= Limit
+         &&  NearLast(T.getLocation());
     });
   }
   assert(!Partition.empty());
 
   // For the consecutive tokens, find the length of the SLocEntry to contain
   // all of them.
   SourceLocation::UIntTy FullLength =
       Partition.back().getEndLoc().getRawEncoding() -

clang/test/Lexer/update_consecutive_macro_crash.cpp

+// RUN: %clang -cc1 -fsyntax-only -verify %s 2>&1
+
+#define X(val2) Y(val2++) // expected-note {{macro 'X' defined here}}
+#define Y(expression) expression ;
+
+void foo() {
+  // https://github.com/llvm/llvm-project/issues/60722:
+  //
+  // - Due to to the error recovery, the 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 takes account
+  //   the written tokens  int{,} , and the extra inserted ) token points to the
+  //    Limit source location which triggered an empty Partition violation.
+  X(int{,}); // expected-error {{too many arguments provided to function-like macro invocation}} \
+                 expected-error {{expected expression}} \
+                 expected-note {{parentheses are required around macro argument containing braced initializer list}}
+}