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

[David Blaikie]

On Fri, Mar 1, 2013 at 3:55 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 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).

I'm concerned that this kind of development approach doesn't actually
make code more robust if we're designing the program to continue down
other untested code paths (OK - so it doesn't infloop here, it does
something else unexpected later). Are clients of this API intending to
account for invalid FileIDs being returned, for example.

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

I agree but don't see how this relates to the issue above. If we need
to be robust to such changes then it should be intended behavior in
Clang and we shouldn't have the assert.

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