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