Page MenuHomePhabricator

[Clang] When using SEH, create a impl instance for CrashRecoveryContext. NFCI.
ClosedPublic

Authored by aganea on Feb 5 2020, 11:00 AM.

Details

Summary

Previously, the SEH codepath in CrashRecoveryContext didn't create a CrashRecoveryContextImpl. The other codepaths (VEH and Unix) were creating it.

When running with -fintegrated-cc1, this is needed to handle exit() as a jump to CrashRecoveryContext's exception filter, through a call to RaiseException. In that situation, we need a user-defined exception code, which is later interpreted as an exit() by the exception filter. Which in turn needs to set RetCode accordingly, inside the exception filter, and before calling HandleCrash().

This is a support patch for D73742.

Diff Detail

Event Timeline

aganea created this revision.Feb 5 2020, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 11:00 AM
aganea retitled this revision from [Clang] When using SEH, create a context for CrashRecoveryContext to [Clang] When using SEH, create a impl instance for CrashRecoveryContext.Feb 5 2020, 11:00 AM
aganea retitled this revision from [Clang] When using SEH, create a impl instance for CrashRecoveryContext to [Clang] When using SEH, create a impl instance for CrashRecoveryContext. NFCI..
rnk accepted this revision.Feb 5 2020, 3:55 PM

Seems fine in principle. lgtm with the one issue fixed.

llvm/lib/Support/CrashRecoveryContext.cpp
216–217

I think optnone has to remain attached to the function using __try, not the filter.

218

So, here.

This revision is now accepted and ready to land.Feb 5 2020, 3:55 PM
hans added a comment.Feb 6 2020, 1:07 AM

lgtm with rnk's comment addressed

This revision was automatically updated to reflect the committed changes.
aganea marked 3 inline comments as done.
aganea added inline comments.Feb 6 2020, 4:28 PM
llvm/lib/Support/CrashRecoveryContext.cpp
216–217

Silly me, of course it's on the function using the __try. Thanks for catching this!

tra added a subscriber: tra.Feb 11 2020, 1:33 PM

Tensorflow folks report that the __try() here does not compile on windows:

ERROR: T:/tmp/nsz6drem/external/llvm-project/llvm/BUILD:3716:1: C++ compilation of rule '@llvm-project//llvm:support' failed (Exit 2)
external/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp(218): error C2712: Cannot use __try in functions that require object unwinding

Any ideas what could be wrong?

In D74078#1870669, @tra wrote:

Tensorflow folks report that the __try() here does not compile on windows:
Any ideas what could be wrong?

Usage of /EHsc most likely. Does it work on the previous revision of the code? If that's the case, could ask them to move the __try/__catch block into a separate function like it used to be, see if that works for them?

rnk added a comment.Feb 11 2020, 3:59 PM

I repro'd this locally with /EHsc and came up with a fix: rGa349c09162a8260bdf691c4f7ab72a16c33975f6

hans added a comment.Feb 12 2020, 2:21 AM
In D74078#1871015, @rnk wrote:

I repro'd this locally with /EHsc and came up with a fix: rGa349c09162a8260bdf691c4f7ab72a16c33975f6

Thanks! Pushed that to 10.x in d8a6deab7a84a559a1ff9f2196dae68870af80bf