Page MenuHomePhabricator

[libclang] Allow crash recovery with LIBCLANG_NOTHREADS
ClosedPublic

Authored by nik on Sep 7 2017, 1:48 AM.

Details

Summary

Enabled crash recovery for some libclang operations on a calling thread even
when LIBCLANG_NOTHREAD is specified.

Previously it would only run under crash recovery if LIBCLANG_NOTHREAD is not
set. Moved handling of LIBCLANG_NOTHREAD env variable into RunSafely from its
call sites.

Event Timeline

nik created this revision.Sep 7 2017, 1:48 AM
nik added a subscriber: cfe-commits.
yvvan edited edge metadata.Sep 7 2017, 2:00 AM

This actually fixes the ability to run safely without threads. This happens because by default this solution leads to try/catch block instead of the direct function call which is implemented in deleted if blocks.

klimek edited edge metadata.

Adding Argyrios, who might have insight on how this is used.
I think this had the wrong list of reviewers so far :(

ilya-biryukov edited edge metadata.Sep 27 2017, 8:53 AM

I think your change makes sense, but maybe asking for a better description.

It appears you:

  • Enabled crash recovery for some libclang operations on a calling thread even when LIBCLANG_NOTHREAD is specified. Previously it would only run under crash recovery if LIBCLANG_NOTHREAD is not set.
  • Moved handling of LIBCLANG_NOTHREAD env variable into RunSafely from its call sites.

That said, I am not familiar with the code you're changing, so can't really LGTM this. Hopefully, someone else can do that.

erikjv added a subscriber: erikjv.
nik updated this revision to Diff 119823.Oct 23 2017, 2:51 AM

Rebased and took over better wording/description from Ilya.

nik edited the summary of this revision. (Show Details)Oct 23 2017, 2:53 AM
nik added a comment.Oct 23 2017, 2:56 AM

Hmm, apparently "arc diff --update D37554" did not take the new commit message into account. Changed it manually with the web interface.

Ilya, I hope it's OK if I take your description :)

That said, I am not familiar with the code you're changing, so can't really LGTM this. Hopefully, someone else can do that.

So who would be the appropriate reviewer here? Manuel, any idea?

In D37554#903401, @nik wrote:

Ilya, I hope it's OK if I take your description :)

Sure, no problem.

...added some more reviewers that I've found with git blame. Ping to the new ones :)

nik added a comment.Nov 10 2017, 3:50 AM

I don't get a review and I also don't know who should I add further to this change. What now?

This revision is now accepted and ready to land.Nov 10 2017, 7:01 AM
erikjv closed this revision.Nov 14 2017, 1:35 AM

Committed as r318142.