On May 24, 2013, at 3:40 PM, Jordan Rose <[email protected]> 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 <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
