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

[Argyrios Kyrtzidis]

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

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

Yes, they should account for invalid FileIDs.

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

The assertion is to hopefully catch & fix & move on a case where the invariant breaks, while using an assertions-enabled build.
The if - return here is to avoid hanging the whole process if the invariant breaks, something that provides more pain than benefit, particularly to the user of the process.

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