- Notifications
You must be signed in to change notification settings - Fork5.1k
Enable new exception handling by default#98570
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/inc/clrconfigvalues.h Outdated
| @@ -259,7 +259,7 @@ RETAIL_CONFIG_DWORD_INFO(UNSUPPORTED_legacyCorruptedStateExceptionsPolicy, W("le | |||
| CONFIG_DWORD_INFO(INTERNAL_SuppressLostExceptionTypeAssert, W("SuppressLostExceptionTypeAssert"), 0, "") | |||
| RETAIL_CONFIG_DWORD_INFO(INTERNAL_UseEntryPointFilter, W("UseEntryPointFilter"), 0, "") | |||
| RETAIL_CONFIG_DWORD_INFO(INTERNAL_Corhost_Swallow_Uncaught_Exceptions, W("Corhost_Swallow_Uncaught_Exceptions"), 0, "") | |||
| RETAIL_CONFIG_DWORD_INFO(EXTERNAL_EnableNewExceptionHandling, W("EnableNewExceptionHandling"),0, "Enable new exception handling."); | |||
| RETAIL_CONFIG_DWORD_INFO(EXTERNAL_EnableNewExceptionHandling, W("EnableNewExceptionHandling"),1, "Enable new exception handling."); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is there a way to configure this via runtimeconfig as well? We had some asks for having even W^X configurable through that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, let me figure out how to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mangod9 I've just added a commit that adds "System.Runtime.EnableNewExceptionHandling" setting that can control that from runtime.config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we not call this "New" exception handling? I've worked on way too many features where the "New" thing was 15 years old and its name confused everyone at all times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, I suppose thats the name throughout the product at this point. Oh well. I suppose New it is. Sigh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@davidwrighton yeah, I know. I couldn't come up with anything better over the time. If you have a better idea, I'd be happy to rename it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a quick idea - would usingDOTNET_ExceptionHandlingMode andSystem.Runtime.ExceptionHandlingMode with a numeric value feel better? We would use 0 for the old EH, 1 for the new EH and if we ever moved it to something else, then we could keep incrementing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume that this setting is temporary just in case somebody gets blocked and we are going to delete it at some point. Do we have an idea when we are going to delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I couldn't come up with anything better over the time.
A better name would beLegacyExceptionHandling with opposite meaning. It makes it clear that it is something that one should not use and that is likely going to disappear at some point.
/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
Looks like there are PALTest failures? |
Yes, break from another PAL refactoring, see#98574 |
I don't know what happened. Huge number of tests are failing. I had a test PR before with all the changes and all tests were passing. I must have merged something incorrectly. I need to investigate it. |
/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
5950a3b to3f53147Compare/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
This change just flips the config knob to enable the new exceptionhandling by default.
From the EnableNewExceptionHandling to LegacyExceptionHandling
3f53147 to918ff7cCompare
This change just flips the config knob to enable the new exception handling by default.