[clangd] Fix HitMapping assertion in Tokens.cpp

[vabridgers]

Summary

Extend test cases for tokens, and remove assertion that is unneeded and
hitting in Tokens.cpp.

Fixes Bugzilla https://bugs.llvm.org/show_bug.cgi?id=45428

[vabridgers]

Remove extraneous test case

[sammccall]

Thanks for tracking this down. Unfortunately I don't think removing the assertion is enough, the code doesn't handle this case correctly.

clang/unittests/Tooling/Syntax/TokensTest.cpp

487

This is the case that always worked, right? Not sure we need to add this test.

505

From the linked bug, the varags isn't essential to this crash.
The linked example can be simplified a little further by removing foo:

#define N 1
#define ID(X) X
#define ID2 ID
ID2(N)

This makes it clearer that the trigger is using a macro in a arg to an "indirect" function macro (i.e. the function macro's name is expanded from another macro).

519

The problem with just removing the assertion is that this behavior is wrong: it's mapping M0(, onto { 42 ; } - i.e. the whole M0 macro expansion. Then all the other mappings are empty. Compare to the baseline case above...

This should be [M0_42, ;_46)] => [{_6, ;_10] I think.

[sammccall]

Oh dear, this seems like a design bug.

TokenBuffer only attempts to record "top-level" expansions, i.e. FOO(BAR(BAZ)) will record the expansion of FOO, and say the tokens FOO ( BAR ( BAZ ) ) were expanded into some other thing in an essentially-opaque way.
The problem is this conflates two notions of "top-level"

• macro use is in main file (this is what the PPCallbacks checks)
• tokens emitted by the expansion are in the final expanded token stream (this is what makes the code correct, and I think what we're asserting)

The second seems more fundamental, so we should try patching the code to avoid the former. But I worry there are subtle assumptions of this scattered through...

[vabridgers]

Thank you for the comments. I'll keep looking at this to find a proper fix, but any hints you may have would be greatly appreciated (debugging tips, strategies, areas of code to focus on). This code is new to me, so I may not be as efficient at tracking this down as yourself or Ilya. Best!

[sammccall]

In D77507#1964309, @vabridgers wrote:

Thank you for the comments. I'll keep looking at this to find a proper fix, but any hints you may have would be greatly appreciated (debugging tips, strategies, areas of code to focus on). This code is new to me, so I may not be as efficient at tracking this down as yourself or Ilya. Best!

Hope you don't mind, I ended up writing up a fix myself. (Well, I got frustrated with TokenBuffer's internals being complicated, and rewrote some of it in a way that makes this bug easy to fix.) D77615 is the proposed fix for this bug, and D77614 is the required refactoring.

FWIW I don't have much great advice about debugging this stuff beyond minimizing the example as far as possible and instrumenting with lots of printfs...

[vabridgers]

ayup, no problem and thanks for the fix. I'll abandon this change. Best!

clang/lib/Tooling/Syntax/Tokens.cpp

 //===- Tokens.cpp - collect tokens from preprocessing ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "clang/Tooling/Syntax/Tokens.h"
private:
   /// (!) SpelledIndex is updated in-place.
   void consumeMapping(TokenBuffer::MarkedFile &File, unsigned EndOffset,
                       unsigned BeginExpanded, unsigned EndExpanded,
                       unsigned &SpelledIndex) {
     // We need to record this mapping before continuing.
     unsigned MappingBegin = SpelledIndex;
     ++SpelledIndex;
 
-    bool HitMapping =
-        tryConsumeSpelledUntil(File, EndOffset + 1, SpelledIndex).hasValue();
-    (void)HitMapping;
-    assert(!HitMapping && "recursive macro expansion?");
+    (void)tryConsumeSpelledUntil(File, EndOffset + 1, SpelledIndex).hasValue();
 
     TokenBuffer::Mapping M;
     M.BeginExpanded = BeginExpanded;
     M.EndExpanded = EndExpanded;
     M.BeginSpelled = MappingBegin;
     M.EndSpelled = SpelledIndex;
 
     File.Mappings.push_back(M);

clang/unittests/Tooling/Syntax/TokensTest.cpp

 //===- TokensTest.cpp -----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 file './input.cpp'
   spelled tokens:
     # define EMPTY # define EMPTY_FUNC ( X ) EMPTY EMPTY_FUNC ( 1 + 2 + 3 )
   mappings:
     ['#'_0, 'EMPTY'_9) => ['<eof>'_0, '<eof>'_0)
     ['EMPTY'_9, 'EMPTY_FUNC'_10) => ['<eof>'_0, '<eof>'_0)
     ['EMPTY_FUNC'_10, '<eof>'_18) => ['<eof>'_0, '<eof>'_0)
 )"},
+      // Baseline case, bugz: https://bugs.llvm.org/show_bug.cgi?id=45428
+      {R"cpp(
+    #define NUM 42
+    #define M2(a1,...) {__VA_ARGS__;}
+    #define M1(a2,...) M2(a2,__VA_ARGS__)
+    void foo(void) { M1(0, NUM); }
+    )cpp",
+       R"(expanded tokens:
+  void foo ( void ) { { 42 ; } ; }
+file './input.cpp'
+  spelled tokens:
+    # define NUM 42 # define M2 ( a1 , ... ) { __VA_ARGS__ ; } # define M1 ( a2 , ... ) M2 ( a2 , __VA_ARGS__ ) void foo ( void ) { M1 ( 0 , NUM ) ; }
+  mappings:
+    ['#'_0, 'void'_30) => ['void'_0, 'void'_0)
+    ['M1'_36, ';'_42) => ['{'_6, ';'_10)
+)"},
+      // Reproducer, bugz: https://bugs.llvm.org/show_bug.cgi?id=45428.
+      // Causes mapping miss when invoking tryConsumeSpelledUntil
+      {R"cpp(
+    #define NUM 42
+    #define M2(a1,...) {__VA_ARGS__;}
+    #define M1(a2,...) M2(a2,__VA_ARGS__)
+    #define M0 M1 
+    void foo(void) { M0(0, NUM); }
+    )cpp",
+       R"(expanded tokens:
+  void foo ( void ) { { 42 ; } ; }
+file './input.cpp'
+  spelled tokens:
+    # define NUM 42 # define M2 ( a1 , ... ) { __VA_ARGS__ ; } # define M1 ( a2 , ... ) M2 ( a2 , __VA_ARGS__ ) # define M0 M1 void foo ( void ) { M0 ( 0 , NUM ) ; }
+  mappings:
+    ['#'_0, 'void'_34) => ['void'_0, 'void'_0)
+    ['M0'_40, 'NUM'_44) => ['{'_6, ';'_10)
+    ['NUM'_44, ')'_45) => [';'_10, ';'_10)
+    [')'_45, ';'_46) => [';'_10, ';'_10)
+)"},
       // File ends with a macro replacement.
       {R"cpp(
     #define FOO 10+10;
     int a = FOO
     )cpp",
        R"(expanded tokens:
   int a = 10 + 10 ;
 file './input.cpp'