r176333 - Add one more sanity check in SourceManager::getFileIDLoaded().

[Argyrios Kyrtzidis]

On Mar 1, 2013, at 3:30 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Mar 1, 2013 at 3:11 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> On Mar 1, 2013, at 10:03 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>>> On Fri, Mar 1, 2013 at 9:10 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>> On Feb 28, 2013, at 19:43 , Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>>> 
>>>>> Author: akirtzidis
>>>>> Date: Thu Feb 28 21:43:33 2013
>>>>> New Revision: 176333
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=176333&view=rev
>>>>> Log:
>>>>> Add one more sanity check in SourceManager::getFileIDLoaded().
>>>>> 
>>>>> 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=176333&r1=176332&r2=176333&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/Basic/SourceManager.cpp (original)
>>>>> +++ cfe/trunk/lib/Basic/SourceManager.cpp Thu Feb 28 21:43:33 2013
>>>>> @@ -863,6 +863,11 @@ FileID SourceManager::getFileIDLoaded(un
>>>>>     return Res;
>>>>>   }
>>>>> 
>>>>> +    // Sanity checking, otherwise a bug may lead to hanging in release build.
>>>>> +    if (LessIndex == MiddleIndex) {
>>>>> +      assert(0 && "binary search missed the entry");
>>>> 
>>>> llvm_unreachable?
>>> 
>>> Or actually just remove the if/return/etc:
>>> 
>>> assert(LessIndex == MiddleIndex)
>>> 
>>> We really don't make a habit of writing asserts with fallbacks "just in case".
>> 
>> ...except when seeing actual infinite loops occurring, in which case you defend against it; particularly when the infinite loops occur in a process using libclang.
> 
> Are we seeing multiple bugs of this kind? (eg: we fixed a bug that hit
> an inf loop due to this case, then later on we hit another, different
> bug that exhibited as an infinite loop here again) If it's just one so
> far: I assume we /fix/ the bug & move on, no?

This is not so easy, there is no reproducible test case that you can just "fix & move on".

For clang, the compiler executable, things are a bit easier in some aspects because generally people wait for building to finish before modifying any source file (and if something bad happens at one clang execution because you modified a file during building, nobody would really care).
But libclang is meant to be used in an IDE where you are constantly editing files, thus files can change at _any_ time (e.g. any point while you are trying to use info from a PCH). We have to be robust and defensive to make sure we recover gracefully at all times.

> 
>> 
>>> 
>>> (unreachable inside the 'if' would have the same 'problem' as removing
>>> the if/adding the obvious assert - in release builds we'd still
>>> optimize away the whole block & "a bug may lead to hanging in release
>>> build")
>>> 
>>>>> +      return FileID();
>>>>> +    }
>>>>>   LessIndex = MiddleIndex;
>>>>> }
>>>>> }
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>> 
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>