r182681 - Add some safety checks in a couple of SourceManager functions.

[Argyrios Kyrtzidis]

On May 24, 2013, at 3:40 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> I see that we're fixing crashes, but why isn't it just part of the precondition for this function that it is never called with an invalid FileID?

It's not checking the parameters, the source locations it received may be fine, but then getDecomposedLoc() may return an invalid FileID because, for example, the location was in a PCH and the file it referred to was removed from the file system *after* we fully loaded the PCH.

> 
> Also, you can't change isBeforeInTranslationUnit that way; the sort's not consistent. Invalid locs should be always before or always after valid locs, or you should assert that this doesn't happen.

You are right that the return value becomes meaningless; maybe add an optional "bool *Invalid" parameter and allow callers to check that ?

> 
> Jordan
> 
> 
> On May 24, 2013, at 15:24 , Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 
>> Author: akirtzidis
>> Date: Fri May 24 17:24:04 2013
>> New Revision: 182681
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=182681&view=rev
>> Log:
>> Add some safety checks in a couple of SourceManager functions.
>> 
>> This is to address crash in rdar://13932308
>> 
>> Modified:
>>    cfe/trunk/lib/Basic/SourceManager.cpp
>> 
>> Modified: cfe/trunk/lib/Basic/SourceManager.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/SourceManager.cpp?rev=182681&r1=182680&r2=182681&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Basic/SourceManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/SourceManager.cpp Fri May 24 17:24:04 2013
>> @@ -1958,6 +1958,9 @@ SourceManager::getMacroArgExpandedLocati
>> 
>> std::pair<FileID, unsigned>
>> SourceManager::getDecomposedIncludedLoc(FileID FID) const {
>> +  if (FID.isInvalid())
>> +    return std::make_pair(FileID(), 0);
>> +
>>   // Uses IncludedLocMap to retrieve/cache the decomposed loc.
>> 
>>   typedef std::pair<FileID, unsigned> DecompTy;
>> @@ -1969,11 +1972,14 @@ SourceManager::getDecomposedIncludedLoc(
>>     return DecompLoc; // already in map.
>> 
>>   SourceLocation UpperLoc;
>> -  const SrcMgr::SLocEntry &Entry = getSLocEntry(FID);
>> -  if (Entry.isExpansion())
>> -    UpperLoc = Entry.getExpansion().getExpansionLocStart();
>> -  else
>> -    UpperLoc = Entry.getFile().getIncludeLoc();
>> +  bool Invalid = false;
>> +  const SrcMgr::SLocEntry &Entry = getSLocEntry(FID, &Invalid);
>> +  if (!Invalid) {
>> +    if (Entry.isExpansion())
>> +      UpperLoc = Entry.getExpansion().getExpansionLocStart();
>> +    else
>> +      UpperLoc = Entry.getFile().getIncludeLoc();
>> +  }
>> 
>>   if (UpperLoc.isValid())
>>     DecompLoc = getDecomposedLoc(UpperLoc);
>> @@ -2033,6 +2039,9 @@ bool SourceManager::isBeforeInTranslatio
>>   std::pair<FileID, unsigned> LOffs = getDecomposedLoc(LHS);
>>   std::pair<FileID, unsigned> ROffs = getDecomposedLoc(RHS);
>> 
>> +  if (LOffs.first.isInvalid() || ROffs.first.isInvalid())
>> +    return false;
>> +
>>   // If the source locations are in the same file, just compare offsets.
>>   if (LOffs.first == ROffs.first)
>>     return LOffs.second < ROffs.second;
>> 
>> 
>> _______________________________________________
>> 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/20130524/ec2a7b35/attachment.html>