r195819 - [libclang] Make sure we don't access past the tokens buffer while token annotation.

Argyrios Kyrtzidis akyrtzi at gmail.com
Wed Nov 27 01:02:52 PST 2013 

On Nov 26, 2013, at 10:57 PM, Alp Toker <alp at nuanti.com> wrote:

> 
> On 27/11/2013 05:50, Argyrios Kyrtzidis wrote:
>> Author: akirtzidis
>> Date: Tue Nov 26 23:50:55 2013
>> New Revision: 195819
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=195819&view=rev
>> Log:
>> [libclang] Make sure we don't access past the tokens buffer while token annotation.
>> 
>> Also disable crash recovery using 'LIBCLANG_DISABLE_CRASH_RECOVERY' environment variable.
>> 
>> Modified:
>>     cfe/trunk/test/Index/annotate-tokens.cpp
>>     cfe/trunk/test/Index/crash-recovery.c
>>     cfe/trunk/tools/libclang/CIndex.cpp
>> 
>> Modified: cfe/trunk/test/Index/annotate-tokens.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/annotate-tokens.cpp?rev=195819&r1=195818&r2=195819&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Index/annotate-tokens.cpp (original)
>> +++ cfe/trunk/test/Index/annotate-tokens.cpp Tue Nov 26 23:50:55 2013
>> @@ -28,6 +28,11 @@ struct TS {
>>  template <bool (*tfn)(X*)>
>>  void TS<tfn>::foo() {}
>>  
>> +void test4() {
>> +  if (int p = 0)
>> +    return;
>> +}
>> +
>>  // RUN: c-index-test -test-annotate-tokens=%s:1:1:30:1 %s -fno-delayed-template-parsing | FileCheck %s
>>  // CHECK: Keyword: "struct" [1:1 - 1:7] StructDecl=bonk:1:8 (Definition)
>>  // CHECK: Identifier: "bonk" [1:8 - 1:12] StructDecl=bonk:1:8 (Definition)
>> @@ -173,3 +178,10 @@ void TS<tfn>::foo() {}
>>  // CHECK: Punctuation: ")" [29:19 - 29:20] CXXMethod=foo:29:15 (Definition)
>>  // CHECK: Punctuation: "{" [29:21 - 29:22] CompoundStmt=
>>  // CHECK: Punctuation: "}" [29:22 - 29:23] CompoundStmt=
>> +
>> +// RUN: env LIBCLANG_DISABLE_CRASH_RECOVERY=1 c-index-test -test-annotate-tokens=%s:32:1:32:13 %s | FileCheck %s -check-prefix=CHECK2
>> +// CHECK2: Keyword: "if" [32:3 - 32:5] IfStmt=
>> +// CHECK2: Punctuation: "(" [32:6 - 32:7] IfStmt=
>> +// CHECK2: Keyword: "int" [32:7 - 32:10] VarDecl=p:32:11 (Definition)
>> +// CHECK2: Identifier: "p" [32:11 - 32:12] VarDecl=p:32:11 (Definition)
>> +// CHECK2: Punctuation: "=" [32:13 - 32:14] VarDecl=p:32:11 (Definition)
>> 
>> Modified: cfe/trunk/test/Index/crash-recovery.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/crash-recovery.c?rev=195819&r1=195818&r2=195819&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Index/crash-recovery.c (original)
>> +++ cfe/trunk/test/Index/crash-recovery.c Tue Nov 26 23:50:55 2013
>> @@ -1,6 +1,7 @@
>>  // RUN: not c-index-test -test-load-source all %s 2> %t.err
>>  // RUN: FileCheck < %t.err -check-prefix=CHECK-LOAD-SOURCE-CRASH %s
>>  // CHECK-LOAD-SOURCE-CRASH: Unable to load translation unit
>> +// RUN: env LIBCLANG_DISABLE_CRASH_RECOVERY=1 not --crash c-index-test -test-load-source all %s
>>  //
>>  // REQUIRES: crash-recovery
>>  
>> 
>> Modified: cfe/trunk/tools/libclang/CIndex.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndex.cpp?rev=195819&r1=195818&r2=195819&view=diff
>> ==============================================================================
>> --- cfe/trunk/tools/libclang/CIndex.cpp (original)
>> +++ cfe/trunk/tools/libclang/CIndex.cpp Tue Nov 26 23:50:55 2013
>> @@ -5075,18 +5075,26 @@ class AnnotateTokensWorker {
>>      unsigned BeforeChildrenTokenIdx;
>>    };
>>    SmallVector<PostChildrenInfo, 8> PostChildrenInfos;
>> -  
>> +
>> +  CXToken &getTok(unsigned Idx) {
>> +    assert(Idx < NumTokens);
>> +    return Tokens[Idx];
>> +  }
>> +  const CXToken &getTok(unsigned Idx) const {
>> +    assert(Idx < NumTokens);
>> +    return Tokens[Idx];
>> +  }
>>    bool MoreTokens() const { return TokIdx < NumTokens; }
>>    unsigned NextToken() const { return TokIdx; }
>>    void AdvanceToken() { ++TokIdx; }
>>    SourceLocation GetTokenLoc(unsigned tokI) {
>> -    return SourceLocation::getFromRawEncoding(Tokens[tokI].int_data[1]);
>> +    return SourceLocation::getFromRawEncoding(getTok(tokI).int_data[1]);
>>    }
>>    bool isFunctionMacroToken(unsigned tokI) const {
>> -    return Tokens[tokI].int_data[3] != 0;
>> +    return getTok(tokI).int_data[3] != 0;
>>    }
>>    SourceLocation getFunctionMacroTokenLoc(unsigned tokI) const {
>> -    return SourceLocation::getFromRawEncoding(Tokens[tokI].int_data[3]);
>> +    return SourceLocation::getFromRawEncoding(getTok(tokI).int_data[3]);
>>    }
>>  
>>    void annotateAndAdvanceTokens(CXCursor, RangeComparisonResult, SourceRange);
>> @@ -5333,7 +5341,7 @@ AnnotateTokensWorker::Visit(CXCursor cur
>>    // This can happen for C++ constructor expressions whose range generally
>>    // include the variable declaration, e.g.:
>>    //  MyCXXClass foo; // Make sure we don't annotate 'foo' as a CallExpr cursor.
>> -  if (clang_isExpression(cursorK)) {
>> +  if (clang_isExpression(cursorK) && MoreTokens()) {
>>      const Expr *E = getCursorExpr(cursor);
>>      if (const Decl *D = getCursorParentDecl(cursor)) {
>>        const unsigned I = NextToken();
>> @@ -5455,14 +5463,23 @@ public:
>>    }
>>  
>>  private:
>> +  CXToken &getTok(unsigned Idx) {
>> +    assert(Idx < NumTokens);
>> +    return Tokens[Idx];
>> +  }
>> +  const CXToken &getTok(unsigned Idx) const {
>> +    assert(Idx < NumTokens);
>> +    return Tokens[Idx];
>> +  }
>> +
>>    SourceLocation getTokenLoc(unsigned tokI) {
>> -    return SourceLocation::getFromRawEncoding(Tokens[tokI].int_data[1]);
>> +    return SourceLocation::getFromRawEncoding(getTok(tokI).int_data[1]);
>>    }
>>  
>>    void setFunctionMacroTokenLoc(unsigned tokI, SourceLocation loc) {
>>      // The third field is reserved and currently not used. Use it here
>>      // to mark macro arg expanded tokens with their expanded locations.
>> -    Tokens[tokI].int_data[3] = loc.getRawEncoding();
>> +    getTok(tokI).int_data[3] = loc.getRawEncoding();
>>    }
>>  };
>>  
>> @@ -6498,6 +6515,11 @@ bool RunSafely(llvm::CrashRecoveryContex
>>                 unsigned Size) {
>>    if (!Size)
>>      Size = GetSafetyThreadStackSize();
>> +  if (getenv("LIBCLANG_DISABLE_CRASH_RECOVERY")) {
>> +    // Don't use crash recovery.
>> +    llvm::llvm_execute_on_thread(Fn, UserData, Size);
>> +    return true;
>> +  }
> 
> Hi Argyrios,
> 
> I don't think this change does what you want. It'll force threaded execution even when not requested.
> 
> How about just:
> 
> --- a/tools/libclang/CIndex.cpp
> +++ b/tools/libclang/CIndex.cpp
> @@ -2544,7 +2544,8 @@ CXIndex clang_createIndex(int excludeDeclarationsFromPCH,
>                            int displayDiagnostics) {
>    // We use crash recovery to make some of our APIs more reliable, implicitly
>    // enable it.
> -  llvm::CrashRecoveryContext::Enable();
> +  if (!getenv("LIBCLANG_DISABLE_CRASH_RECOVERY"))
> +    llvm::CrashRecoveryContext::Enable();
>  
>    // Enable support for multithreading in LLVM.
>    {
> 
> Given that RunSafely() is documented to "execute the given code safely" I think this would also be more self-documenting.
> 

That's much better; in r195829, thanks!

> Alp.
> 
> 
>>    if (Size)
>>      return CRC.RunSafelyOnThread(Fn, UserData, Size);
>>    return CRC.RunSafely(Fn, UserData);
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> -- 
> http://www.nuanti.com
> the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131127/36b31fe1/attachment.html>

More information about the cfe-commits mailing list