[cfe-commits] r141048 - /cfe/trunk/lib/Basic/SourceManager.cpp

[Douglas Gregor]

On Oct 3, 2011, at 9:49 PM, Argyrios Kyrtzidis wrote:

> On Oct 3, 2011, at 5:03 PM, Eli Friedman wrote:
> 
>> On Mon, Oct 3, 2011 at 4:43 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> Author: akirtzidis
>>> Date: Mon Oct  3 18:43:01 2011
>>> New Revision: 141048
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=141048&view=rev
>>> Log:
>>> Make sure SourceManager::getFileIDLoaded doesn't hang in release build because of invalid passed parameter.
>>> rdar://10210140
>>> 
>>> 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=141048&r1=141047&r2=141048&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Basic/SourceManager.cpp (original)
>>> +++ cfe/trunk/lib/Basic/SourceManager.cpp Mon Oct  3 18:43:01 2011
>>> @@ -732,6 +732,10 @@
>>> FileID SourceManager::getFileIDLoaded(unsigned SLocOffset) const {
>>>  assert(SLocOffset >= CurrentLoadedOffset && "Bad function choice");
>>> 
>>> +  // Sanity checking, otherwise a bug may lead to hanging in release build.
>>> +  if (SLocOffset < CurrentLoadedOffset)
>>> +    return FileID();
>>> +
>>>  // Essentially the same as the local case, but the loaded array is sorted
>>>  // in the other direction.
>> 
>> Having an assert followed by an if statement checking the exact same
>> condition doesn't really make sense: it means that either the assert
>> is incorrect or the check is useless.
> 
> Talked to Eli; the idea here is that if the invariant breaks the assertion should trigger, but in case there is another bug we should really really not hang in release. (The underlying bug for the hang in rdar://10210140 is already fixed in r138813).


Expanding on this a bit… the context is IDE integration via libclang. We follow this approach only for the critical parts of libclang that can't run under crash recovery, such as manipulating source locations, exploring the AST, or deserializing data from an AST file. With parsing and semantic analysis, and nearly all other areas of the compiler, we rely on assertions to catch problems (and libclang's crash recovery mechanisms when things really go downhill).

	- Doug