r183451 - [analyzer] Ensure that pieces with invalid locations always get removed from the BugReport

[Anna Zaks]

Thanks,
Fixed in r183455.

On Jun 6, 2013, at 3:11 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Jun 6, 2013, at 15:02 , Anna Zaks <ganna at apple.com> wrote:
> 
>> Author: zaks
>> Date: Thu Jun  6 17:02:58 2013
>> New Revision: 183451
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=183451&view=rev
>> Log:
>> [analyzer] Ensure that pieces with invalid locations always get removed from the BugReport
>> 
>> The function in which we were doing it used to be conditionalized. Add a new unconditional
>> cleanup step.
>> 
>> This fixes PR16227 (radar://14073870) - a crash when generating html output for one of the test files.
>> 
>> Modified:
>>    cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
>>    cfe/trunk/test/Analysis/unix-fns.c
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=183451&r1=183450&r2=183451&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Thu Jun  6 17:02:58 2013
>> @@ -162,13 +162,6 @@ static bool removeUnneededCalls(PathPiec
>>     IntrusiveRefCntPtr<PathDiagnosticPiece> piece(pieces.front());
>>     pieces.pop_front();
>> 
>> -    // Throw away pieces with invalid locations. Note that we can't throw away
>> -    // calls just yet because they might have something interesting inside them.
>> -    // If so, their locations will be adjusted as necessary later.
>> -    if (piece->getKind() != PathDiagnosticPiece::Call &&
>> -        piece->getLocation().asLocation().isInvalid())
>> -      continue;
>> -
>>     switch (piece->getKind()) {
>>       case PathDiagnosticPiece::Call: {
>>         PathDiagnosticCallPiece *call = cast<PathDiagnosticCallPiece>(piece);
>> @@ -218,8 +211,7 @@ static bool hasImplicitBody(const Decl *
>> }
>> 
>> /// Recursively scan through a path and make sure that all call pieces have
>> -/// valid locations. Note that all other pieces with invalid locations should
>> -/// have already been pruned out.
>> +/// valid locations. 
>> static void adjustCallLocations(PathPieces &Pieces,
>>                                 PathDiagnosticLocation *LastCallLocation = 0) {
>>   for (PathPieces::iterator I = Pieces.begin(), E = Pieces.end(); I != E; ++I) {
>> @@ -252,6 +244,26 @@ static void adjustCallLocations(PathPiec
>>   }
>> }
>> 
>> +/// Remove all pieces with invalid locations as these cannot be serialized.
>> +/// We might have pieces with invalid locations as a result of inlining Body
>> +/// Farm generated functions.
>> +static void removePiecesWithInvalidLocations(PathPieces &Pieces) {
>> +  for (PathPieces::iterator I = Pieces.begin(), E = Pieces.end(); I != E; ++I) {
>> +    if (PathDiagnosticCallPiece *C = dyn_cast<PathDiagnosticCallPiece>(*I))
>> +      removePiecesWithInvalidLocations(C->path);
>> +
>> +    if (PathDiagnosticMacroPiece *M = dyn_cast<PathDiagnosticMacroPiece>(*I))
>> +      removePiecesWithInvalidLocations(M->subPieces);
>> +
>> +    if (!(*I)->getLocation().isValid() ||
>> +        !(*I)->getLocation().asLocation().isValid()) {
>> +      Pieces.erase(I);
> 
> I is not valid after this erase(), so ++I could crash. The right thing to do here is "I = Pieces.erase(I)" (and then not increment I).
> 
>> +      continue;
>> +    }
>> +    
>> +  }
>> +}
>> +
>> //===----------------------------------------------------------------------===//
>> // PathDiagnosticBuilder and its associated routines and helper objects.
>> //===----------------------------------------------------------------------===//
>> @@ -3151,8 +3163,11 @@ bool GRBugReporter::generatePathDiagnost
>>         (void)stillHasNotes;
>>       }
>> 
>> +      // Redirect all call pieces to have valid locations.
>>       adjustCallLocations(PD.getMutablePieces());
>> 
>> +      removePiecesWithInvalidLocations(PD.getMutablePieces());
>> +
>>       if (ActiveScheme == PathDiagnosticConsumer::AlternateExtensive) {
>>         SourceManager &SM = getSourceManager();
>> 
>> 
>> Modified: cfe/trunk/test/Analysis/unix-fns.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/unix-fns.c?rev=183451&r1=183450&r2=183451&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/unix-fns.c (original)
>> +++ cfe/trunk/test/Analysis/unix-fns.c Thu Jun  6 17:02:58 2013
>> @@ -1,6 +1,6 @@
>> // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,unix.API,osx.API %s -analyzer-store=region -analyzer-output=plist -analyzer-eagerly-assume -analyzer-config faux-bodies=true -analyzer-config path-diagnostics-alternate=false -fblocks -verify -o %t.plist
>> // RUN: FileCheck --input-file=%t.plist %s
>> -
>> +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.API,osx.API -analyzer-output=html -analyzer-config faux-bodies=true -fblocks -o %T/dir %s
> 
> This should use a variant on %t, not a subfolder in %T, so that the test's name is included in the temporary directory. We've used "%t.dir" before.
> 
> 
>> struct _opaque_pthread_once_t {
>>   long __sig;
>>   char __opaque[8];
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130606/933328fa/attachment.html>