[PATCH] D15173: [Preprocessor] Fix assertion in AnnotatePreviousCachedTokens

[Bruno Cardoso Lopes via cfe-commits]

bruno updated this revision to Diff 44180.
bruno added a comment.

Hi Richard,

Thanks for the comments. Updated the patch!

In http://reviews.llvm.org/D15173#313235, @rsmith wrote:

> I think that this will leave us with a broken token stream. In your example, the cached token stream starts as
>
>   `NSArray` `<` `id` `<` `PB` `>>` `*` [...]
>   
>
> ... and we try to annotate the `id<PB>` with our `CachedLexPos` pointing at the `*` token. That leaves `CachedTokens` containing:
>
>   `NSArray` `<` `(type annotation)` `*` [...]
>   
>
> ... which is wrong. We need to actually convert the `tok::greatergreater` in `CachedTokens` into a `tok::greater` and update its location and length in order for the cached token stream to be correctly updated. Otherwise if the parser backtracks it will see the wrong token stream.


I don't think this happens, the type annotation starts at 7:11 and ends at 7:24:
identifier 'NSArray'            Loc=<testcase.mm:7:11>
greatergreater '>>'             Loc=<testcase.mm:7:24>

The code that follows the assert then patches the CachedTokens the correct way, see the CachedTokens before and after:

- Before:

(clang::Preprocessor::CachedTokensTy) $32 = {

  [0] = (Loc = 89, UintData = 1, PtrData = 0x0000000000000000, Kind = l_paren, Flags = 0)
  [1] = (Loc = 90, UintData = 7, PtrData = 0x000000010d82fba0, Kind = identifier, Flags = 0)
  [2] = (Loc = 97, UintData = 1, PtrData = 0x0000000000000000, Kind = less, Flags = 0)
  [3] = (Loc = 98, UintData = 2, PtrData = 0x000000010d01da58, Kind = identifier, Flags = 0)
  [4] = (Loc = 100, UintData = 1, PtrData = 0x0000000000000000, Kind = less, Flags = 0)
  [5] = (Loc = 101, UintData = 2, PtrData = 0x000000010d82fb70, Kind = identifier, Flags = 0)
  [6] = (Loc = 103, UintData = 2, PtrData = 0x0000000000000000, Kind = greatergreater, Flags = 0)
  [7] = (Loc = 106, UintData = 1, PtrData = 0x0000000000000000, Kind = star, Flags = 2)

}

- After:

(clang::Preprocessor::CachedTokensTy) $34 = {

  [0] = (Loc = 89, UintData = 1, PtrData = 0x0000000000000000, Kind = l_paren, Flags = 0)
  [1] = (Loc = 90, UintData = 104, PtrData = 0x000000010d820660, Kind = annot_typename, Flags = 0)
  [2] = (Loc = 106, UintData = 1, PtrData = 0x0000000000000000, Kind = star, Flags = 2)

}


http://reviews.llvm.org/D15173

Files:
  lib/Lex/PPCaching.cpp
  test/Parser/objcxx11-protocol-in-template.mm

Index: test/Parser/objcxx11-protocol-in-template.mm
===================================================================
--- test/Parser/objcxx11-protocol-in-template.mm
+++ test/Parser/objcxx11-protocol-in-template.mm
@@ -8,3 +8,11 @@
 
 vector<id<P>> v;
 vector<vector<id<P>>> v2;
+
+ at protocol PA;
+ at protocol PB;
+
+ at class NSArray<ObjectType>;
+typedef int some_t;
+
+id<PA> FA(NSArray<id<PB>> *h, some_t group);
Index: lib/Lex/PPCaching.cpp
===================================================================
--- lib/Lex/PPCaching.cpp
+++ lib/Lex/PPCaching.cpp
@@ -97,13 +97,33 @@
 void Preprocessor::AnnotatePreviousCachedTokens(const Token &Tok) {
   assert(Tok.isAnnotation() && "Expected annotation token");
   assert(CachedLexPos != 0 && "Expected to have some cached tokens");
-  assert(CachedTokens[CachedLexPos-1].getLastLoc() == Tok.getAnnotationEndLoc()
-         && "The annotation should be until the most recent cached token");
+
+#ifndef NDEBUG
+  {
+    Token CachedLastTok = CachedTokens[CachedLexPos - 1];
+    SourceLocation CachedLastTokLoc = CachedLastTok.getLastLoc();
+    SourceLocation TokAnnEndLoc = Tok.getAnnotationEndLoc();
+
+    // The annotation should be until the most recent cached token. Since
+    // `Tok` length could be bigger than one (e.g. greatergreater '>>'), account
+    // for that case before checking the assertion.
+    if (CachedLastTokLoc != TokAnnEndLoc && !CachedLastTok.isAnnotation()) {
+      CachedLastTokLoc =
+          CachedLastTokLoc.getLocWithOffset(CachedLastTok.getLength());
+      unsigned TokAnnEndLocSize = Lexer::MeasureTokenLength(
+          SourceMgr.getSpellingLoc(TokAnnEndLoc), SourceMgr, LangOpts);
+      TokAnnEndLoc = TokAnnEndLoc.getLocWithOffset(TokAnnEndLocSize);
+    }
+
+    assert(CachedLastTokLoc == TokAnnEndLoc &&
+           "The annotation should be until the most recent cached token");
+  }
+#endif
 
   // Start from the end of the cached tokens list and look for the token
   // that is the beginning of the annotation token.
   for (CachedTokensTy::size_type i = CachedLexPos; i != 0; --i) {
-    CachedTokensTy::iterator AnnotBegin = CachedTokens.begin() + i-1;
+    CachedTokensTy::iterator AnnotBegin = CachedTokens.begin() + i - 1;
     if (AnnotBegin->getLocation() == Tok.getLocation()) {
       assert((BacktrackPositions.empty() || BacktrackPositions.back() < i) &&
              "The backtrack pos points inside the annotated tokens!");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D15173.44180.patch
Type: text/x-patch
Size: 2445 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160107/181ed544/attachment.bin>