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

[Argyrios Kyrtzidis via cfe-commits]

Hi Bruno,

> On Dec 2, 2015, at 7:29 PM, Bruno Cardoso Lopes <bruno.cardoso at gmail.com> wrote:
> 
> bruno created this revision.
> bruno added reviewers: doug.gregor, akyrtzi.
> bruno added subscribers: cfe-commits, dexonsmith.
> 
> Consider the following ObjC++ snippet:
> 
>  @protocol PA;
>  @protocol PB;
> 
>  @class NSArray<ObjectType>;
>  typedef int some_t;
> 
>  id<PA> FA(NSArray<id<PB>> *h, some_t group);
> 
> This would hit an assertion in the parser after generating an annotation token while
> trying to update the token cache:
> 
> Assertion failed: (CachedTokens[CachedLexPos-1].getLastLoc() == Tok.getAnnotationEndLoc() && "The annotation should be until the most recent cached token")
> ...
> 7  clang::Preprocessor::AnnotatePreviousCachedTokens(clang::Token const&) + 494
> 8  clang::Parser::TryAnnotateTypeOrScopeTokenAfterScopeSpec(bool, bool, clang::CXXScopeSpec&, bool) + 1163
> 9  clang::Parser::TryAnnotateTypeOrScopeToken(bool, bool) + 361
> 10 clang::Parser::isCXXDeclarationSpecifier(clang::Parser::TPResult, bool*) + 598
> ...
> 
> The cached preprocessor token in this case is:
> 
> greatergreater '>>'             Loc=<testcase.mm:7:24> 
> 
> while the annotation ("NSArray<id<PB>>") ends at "testcase.mm:7:25", hence the assertion.
> The mismatch only happens because of the cached token length and the assertion should account for that.
> 
> 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,8 +97,19 @@
> 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");
> +
> +  // The annotation should be until the most recent cached token. Since
> +  // `Tok` length could be bigger than one (e.g. greatergreater '>>'), account
> +  // for that cases before checking the assertion.
> +  Token CachedLastTok = CachedTokens[CachedLexPos - 1];
> +  unsigned CachedLastTokLoc = CachedLastTok.getLastLoc().getRawEncoding();
> +  unsigned TokAnnEndLoc = Tok.getAnnotationEndLoc().getRawEncoding();
> +  if (CachedLastTokLoc != TokAnnEndLoc && !CachedLastTok.isAnnotation())
> +    CachedLastTokLoc += CachedLastTok.getLength() - 1;
> +  (void)CachedLastTokLoc;
> +  (void)TokAnnEndLoc;
> +  assert(CachedLastTokLoc == TokAnnEndLoc &&
> +         "The annotation should be until the most recent cached token");
> 
>   // Start from the end of the cached tokens list and look for the token
>   // that is the beginning of the annotation token.

This would be better to be wrapped inside a "#ifndef NDEBUG” section.
Also the above is a bit ad-hoc, not quite general.

How about doing something like:

Token CachedLastTok = CachedTokens[CachedLexPos - 1];
unsigned CachedLastTokLoc = CachedLastTok.getLastLoc().getLocWithOffset(CachedLastTok.getLength())

unsigned TokAnnEndLoc =  Tok.getAnnotationEndLoc();
unsigned TokAnnEndLocSize =  Lexer:::MeasureTokenLength(SourceMgr.getSpellingLoc(Tok.getAnnotationEndLoc()))
TokAnnEndLocSize = TokAnnEndLoc.getLocWithOffset(TokAnnEndLocSize)


and compare the locations ? The above tries to account for possible different token sizes but the end locations should be the same.


> 
> 
> <D15173.41709.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151207/2f1faba0/attachment.html>