r176333 - Add one more sanity check in SourceManager::getFileIDLoaded().
Argyrios Kyrtzidis
akyrtzi at gmail.com
Sat Mar 2 11:44:15 PST 2013
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>
More information about the cfe-commits
mailing list