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

[Argyrios Kyrtzidis]

On Mar 2, 2013, at 11:00 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Sat, Mar 2, 2013 at 10:57 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> On Mar 2, 2013, at 10:42 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> 
>>>> libclang from the beginning has chosen to avoid crashing the process, see llvm::CrashRecoveryContext.
>>> 
>>> /// This class implements support for running operations in a safe context so
>>> /// that crashes (memory errors, stack overflow, assertion violations) can be
>>> /// detected and control restored to the crashing thread. Crash detection is
>>> /// purely "best effort", the exact set of failures which can be recovered from
>>> /// is platform dependent.
>>> 
>>> This looks like exactly what we need. Instead of doing
>>> 
>>> if (!foo) {
>>> assert(0...);
>>> }
>>> 
>>> You can cause a "crash" and let the crash recovery mechanism handle it.
>> 
>> Doing that will still ignore the crash, but this time with a recovery mechanism that, while good intentioned (not bringing down the process) it's disruptive and should really not be triggered intentionally.
> 
> It's not intentional - if it was we wouldn't have the assert there &
> this would just be an intended/valid codepath.

I mean if the condition for the assertion occurs, triggering the crash recovery mechanism explicitly is bad.

Also, returning FileID() because something failed is a valid codepath there. A loaded source location entry may be invalid because the associated file was removed from the file system, which is why this check exists:

    if (E.getOffset() == 0)
      return FileID(); // invalid entry.


> 
> If we have some way to take "we're in a state we don't expect to be
> in" (& importantly: haven't tested) & turn that in to a well defined
> crash/error reporting that seems to be the right thing to have here.


In a dogfooding situation an assertions-enabled build can be used along with disabling crash recovery.
In a release build for users, recovering as gracefully as possible and not taking down the process is deemed more important.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130302/f4794444/attachment.html>