On Mar 1, 2013, at 4:16 PM, David Blaikie <[email protected]> wrote:
> On Fri, Mar 1, 2013 at 3:55 PM, Argyrios Kyrtzidis <[email protected]> wrote: >> On Mar 1, 2013, at 3:30 PM, David Blaikie <[email protected]> wrote: >> >>> On Fri, Mar 1, 2013 at 3:11 PM, Argyrios Kyrtzidis <[email protected]> >>> wrote: >>>> On Mar 1, 2013, at 10:03 AM, David Blaikie <[email protected]> wrote: >>>> >>>>> On Fri, Mar 1, 2013 at 9:10 AM, Jordan Rose <[email protected]> wrote: >>>>>> >>>>>> On Feb 28, 2013, at 19:43 , Argyrios Kyrtzidis <[email protected]> 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 >>>>>>> [email protected] >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
