Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement native exception handling for PAL #308

Merged
merged 5 commits into from Feb 20, 2015

Conversation

@janvorli
Copy link
Member

@janvorli janvorli commented Feb 20, 2015

This change removes the preexisting exception handling in PAL that was simulating Windows
SEH using a lot of low level machinery. The only remaining part is the code that raises
the exception and that extracts the exception context information.
The PAL_TRY, PAL_EXCEPT, PAL_FINALLY, ... macros are reimplemented using plain C++ exception
handling.

This change removes the preexisting exception handling in PAL that was simulating Windows
SEH using a lot of low level machinery. The only remaining part is the code that raises
the exception and that extracts the exception context information.
The PAL_TRY, PAL_EXCEPT, PAL_FINALLY, ... macros are reimplemented using plain C++ exception
handling.
@janvorli
Copy link
Member Author

@janvorli janvorli commented Feb 20, 2015

@jkotas, @sergiy-k Could you please take a look?

@janvorli
Copy link
Member Author

@janvorli janvorli commented Feb 20, 2015

@kangaroo It seems we will not need to do anything OSX specific, that's why I have made this change in my local branch. If you think there will be some OSX specific changes needed, please let me know.

// non-NULL TRUE An exception escaped from the try block,
// and the filter wanted to handle it.
//

#define DEBUG_OK_TO_RETURN_BEGIN(arg)
#define DEBUG_OK_TO_RETURN_END(arg)

This comment has been minimized.

@jkotas

jkotas Feb 20, 2015
Member

It does not seem to be used anymore in pal.h - delete?

{ \
tryBlock(__param); \
} \
catch (PAL_SEHException& ex) \

This comment has been minimized.

@jkotas

jkotas Feb 20, 2015
Member

I think we should catch(...) in addition to catching (PAL_SEHException) - to match the Windows behavior. Of course, we are not going to have CONTEXT* for the non-SEHException. NULL CONTEXT* should be ok for most cases, if not all.

This comment has been minimized.

@janvorli

janvorli Feb 20, 2015
Author Member

As per our offline discussion, I'll keep it as it is.

{ \
try \
{ \
tryBlock(__param); \

This comment has been minimized.

@jkotas

jkotas Feb 20, 2015
Member

Cosmetic - extra spaces between ; and \

This comment has been minimized.

@janvorli

janvorli Feb 20, 2015
Author Member

Oops, seems like VS has reformatted that piece of code under my hands. I had all the backslashes aligned to the same column.

@jkotas
Copy link
Member

@jkotas jkotas commented Feb 20, 2015

LGTM modulo comments

janvorli added 2 commits Feb 20, 2015
Remove few classes / macros from pal.h that are not used anymore.
Fix some line continuation backslash identations.
With the previous commit, I haven't noticed that the dummy PAL_CheckVirtualUnwind() is now
missing in the release build. Adding it back.
@kangaroo
Copy link

@kangaroo kangaroo commented Feb 20, 2015

@janvorli Great, I'll try to take a look today and let you know.

@pgavlin
Copy link

@pgavlin pgavlin commented Feb 20, 2015

@janvorli Let me make sure I've correctly understood the effect that this will have on the CoreCLR ABI/API:

  • PAL_TryExcept is dead, long live C++ try/catch
  • RaiseException remains s.t. SEH-style information can be provided to the runtime in the thrown exception

So catching exceptions thrown across the interface just got simpler, but throwing exceptions that the runtime is expected to handle remains the same (for a consumer of the API exported by the runtime), correct"?

@janvorli
Copy link
Member Author

@janvorli janvorli commented Feb 20, 2015

@pgavlin I see the change just as an internal implementation detail that the runtime or jit developers should not care about. The code should still use PAL_TRY / PAL_EXCEPT / PAL_EXCEPT_FILTER / PAL_FINALLY and PAL_ENDTRY to catch exceptions or handle cleanup and RaiseException to raise exceptions. The same way as it works on Windows.

@pgavlin
Copy link

@pgavlin pgavlin commented Feb 20, 2015

I don't think that the EH style can be considered an implementation detail, as it's fundamental to the way the runtime communicates with its consumers (hosts, JITs, etc.). Changing the EH style is a potentially breaking change even when the machinery is hidden by APIs (and I personally don't think macros are APIs, exactly). (e.g. if my consumer used C++ exceptions internally, I may now see exceptions from the runtime in my handlers).

I like this change--I just want to make sure that the implications are fully understood.

@kangaroo
Copy link

@kangaroo kangaroo commented Feb 20, 2015

@janvorli Do you have a testcase you've been verifying against on linux?

@janvorli
Copy link
Member Author

@janvorli janvorli commented Feb 20, 2015

@kangaroo I was verifying it just with local testing pieces of code. But it is a good point, I can see now that we have a bunch of exception handling tests in the PAL that are currently disabled, so I am going to enable ones that make sense.

@kangaroo
Copy link

@kangaroo kangaroo commented Feb 20, 2015

@janvorli Great, that'll help me verify parity. Thanks!

{
public:
void SuppressRelease() {}
};
#endif // __cplusplus

#define PAL_TRY(__ParamType, __paramDef, __paramRef) \

This comment has been minimized.

@sergiy-k

sergiy-k Feb 20, 2015

Could you please add a bit more comments describing all of these macros, their parameters and usage? Thank you.

janvorli added 2 commits Feb 20, 2015
This change just adds comments to those methods.
I am removing few other things from the seh-unwind.cpp. First, the code that verified the
stack unwindibility actually couldn't work since in most cases, it would reach a managed
frame and then get screwed. It happened to few people in the recent past and they
were mislead into thinking that it is a problem of the coreclr unwinder.
There is no reasonable way to find out whether a frame is managed or native inside PAL,
that's why I am removign that stuff.
Also, there was now unused vectored exception handling stuff that I am also removing.
Another change is in the pal.h / pal.cpp where the type of the argv parameter was
const char **argv, which prevented passing in char** (the compiler was ok with it in C,
but failed in C++). I have changed it to const char * const *argv, which allows
passing in the char** as well.
@janvorli
Copy link
Member Author

@janvorli janvorli commented Feb 20, 2015

@jkotas, @sergiy-k, I've done some little additional cleanup, could you please take a look to the last checkin?

@sergiy-k
Copy link

@sergiy-k sergiy-k commented Feb 20, 2015

LGTM. :)

janvorli added a commit that referenced this pull request Feb 20, 2015
Reimplement native exception handling for PAL
@janvorli janvorli merged commit ed8692f into dotnet:master Feb 20, 2015
1 check passed
1 check passed
@dotnet-bot
default Merged build finished.
Details
@janvorli janvorli deleted the janvorli:exception-handling branch Feb 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants