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

[David Blaikie]

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?

>
>>
>> (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
>