[cfe-commits] r140131 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h lib/StaticAnalyzer/Core/PathDiagnostic.cpp

[Anna Zaks]

On Sep 19, 2011, at 11:27 PM, Ted Kremenek wrote:

> On Sep 19, 2011, at 6:51 PM, Anna Zaks wrote:
> 
>> +  FullSourceLoc genLocation(const ParentMap *PM=0) const;
>> +  PathDiagnosticRange genRange(const ParentMap *PM=0) const;
> 
> 
> Hi Anna,
> 
> Instead of passing the ParentMap, how about having genLocation() lazily query LocationContext for one?  That way it only gets created when we need it.
> 
> Actually, it it looks like we are still using the LC member variable in genRange(), instead of the passed ParentMap:
> 
>         case Stmt::ObjCForCollectionStmtClass: {
>           SourceLocation L = getValidSourceLocation(S, LC->getParentMap());
>           return SourceRange(L, L);
> 
> Since LocationContext isn't guaranteed to live for the lifetime of PathDiagnosticLocation, I suggest:
> 
> 1) Remove the LC member variable (fix potential use-after-free).

There was still a dependency remaining. Removed in r140146. 

> 2) Pass (possibly null?) LocationContext * to genLocation() and genRange()

Thanks for catching this! LocationContext's constructor is protected so we cannot construct one from AnalysisContext directly (passing ParentMap would solve the issue, I forgot that it's constructed on demand). We could require either LocationContextManager or AnalysisManager to be passed in by the users that do not have LocationContext. AnalysisManager is probably better since that's what is passed to the checkers.

> 3) Lazily query that LocationContext for a ParentMap as needed.  LocationContext (or really, AnalysisContext) will make sure the ParentMap is constructed once.
> 

Will do.

> What do you think?
> 
> Cheers,
> Ted

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