Deprecate DISABLE_EXCEPTION_CATCHING=2 #13733

sbc100 · 2021-03-22T18:55:19Z

We can instead imply this mode by the presence of the
EXCEPTION_CATCHING_ALLOWED list.

This turns DISABLE_EXCEPTION_CATCHING into a binary option
which is easier to deal with both internally and externally.

ChangeLog.md

emcc.py

src/settings.js

ChangeLog.md

aheejin · 2021-03-22T21:42:08Z

emcc.py

@@ -1578,6 +1578,14 @@ def default_setting(name, new_default):
      default_setting('ERROR_ON_UNDEFINED_SYMBOLS', 0)
      default_setting('WARN_ON_UNDEFINED_SYMBOLS', 0)

+    if shared.Settings.EXCEPTION_CATCHING_ALLOWED:
+      if 'DISABLE_EXCEPTION_CATCHING' in settings_key_changes:


How is this line different from

if shared.Settings.DISABLE_EXCEPTION_CATCHING:

? If they are different, should we also check if both shared.Settings.DISABLE_EXCEPTION_CATCHING and shared.Settings.EXCEPTION_CATCHING_ALLOWE are set?

This is set if the user specified it explicitly on the command line (regardless of if the value matches the default).

Doing that here allows me to show a warning (or error) if the user ever passed both these options at the same time.

Then shouldn't we check if both of those two flags are specified in

One in Settings and the other in the command line (and vice versa)

Both in Settings (People can modify settings.js)

Both in the command line

Maybe I could be mistaken.

In theory yes, but I don't think we are careful about local modifications to settings.js. That is, users should use the commandline. Changes to settings.js by an emscripten developer would assume they know the risk of changing internals.

If users are supposed to use the command line, then we should check the case when both of them are in settings_key_change at least?

Also most of these checking lines in this function directly refer to not settings_key_change but shared.Settings directly, which is weird if users are supposed use the command line.

In general yes, although I think this code covers the cases we care about.

I don't think (2) is much of an issue. I don't think folks should be directly modifying settings.js, at least not without also also modifying code like this that is somewhat aware of the default. So I think its fair to say that if EXCEPTION_CATCHING_ALLOWED is set then its the user that set it.

That means if these two conditions are both two they specified both on the command line. That is exactly the case we care about erroring/warning for. I agree we could possibly be more robust here and not make assumptions about the default values.

I added a comment which should hopefully make it a little more clear what is going on.

Sorry, what I was asking is, if we only care about the command line,
why are we not checking settings_key_change for EXCEPTION_CATCHING_ALLOWED as we do for DISABLE_EXCEPTION_CATCHING? We are checking settings_key_changes for one and checking shared.Settings for the other. This is the part I don't understand.

Also, if we only care about command lines, why are all other lines in this function directly checking shared.Settings and now settings_key_changes?

If users are supposed to use the command line, then we should check the case when both of them are in settings_key_change at least?

Also most of these checking lines in this function directly refer to not settings_key_change but shared.Settings directly, which is weird if users are supposed use the command line.

Technically you are correct yes, we should also check for EXCEPTION_CATCHING_ALLOW in settings_key_changes.

Maybe this latest version is more explicit.

Thanks, but the problem here is my lack of knowledge of the code around here ;( Sorry. Can someone answer why we check shared.Settings directly in the code within this function, if we expect the users to use only the command line? I asked why we were not using settings_key_changes for both settings, but other code around this line all check shared.Settings directly, so I'm wondering why this should be, or if this should be, different, and how all this works.

When are we merging settings_key_changes to shared.Settings? Before running this code or after? If after, checking settings.Changes only makes sense, but if so you wouldn't have checked settings_key_changes in the first place...

Sorry for many questions. If taking offline is easier it's fine too.

shared.Settings reflects that actual current settings which is the sum of:

a) the defaults
b) any user command line settings
c) any changes made by the running of the code here (which updates a lot of settings).

In almost all cases shared.Settings is the only thing we care about because it represent the actual current setup.

settings_key_changes is for cases were we actually care about what the user specified on the commandline as opposed to (a) or (c). Its mostly used for diagnostics stuff like this. For example in this case we need to use it so we can tell the different between the default =0 and the command line =0.

I will make a couple of PRs to clean this up a bit and give settings_key_changes a better name :)

emcc.py

src/parseTools.js

src/settings.js

aheejin · 2021-03-22T21:48:27Z

tests/test_core.py

@@ -1236,15 +1236,16 @@ def test_exceptions_3(self):
    self.do_run('src.js', 'Caught exception: Hello\nDone.', args=['2'], no_build=True)

  def test_exceptions_allowed(self):
-    self.set_setting('DISABLE_EXCEPTION_CATCHING', 2)


Shouldn't we set it to 0 instead and not delete the line? The same for other functions.

No because we set EXCEPTION_CATCHING_ALLOWED below and setting them both at the same time is a warning (and we are building with -Werror)

No I meant 0, not 1. The default setting for DISABLE_EXCEPTION_CATCHING is 1, so I thought we needed to turn it off to catch exceptions. Settings DISABLE_EXCEPTION_CATCHING == 0 and EXCEPTION_CATCHING_ALLOWED at the same time is an error?

Or does setting EXCEPTION_CATCHING_ALLOWED automatically imply DISABLE_EXCEPTION_CATCHING == 0 or something?

Yes exactly, setting EXCEPTION_CATCHING_ALLOWED now implies DISABLE_EXCEPTION_CATCHING.

Also setting DISABLE_EXCEPTION_CATCHING to anything, including 0, at the same time as EXCEPTION_CATCHING_ALLOWED an error or warning (=2 is a warning =1, or =0 is an error). They should never appear together .. although we need to continue to support =2 for backward compat (we give a warning in this case).

I see. But why is setting DISABLE_EXCEPTION_CATCHING to 0 also an error? Are gonna do that internally anyway?

It seems cleaner to me that they should never be specified together. Don't you think they should be 100% mutually exclusive?

That fact that internally we set DISABLE_EXCEPTION_CATCHING to zero when EXCEPTION_CATCHING_ALLOWED is specified is kind of an implementation detail I think.

True, but DISABLE_EXCEPTION_CATCHING is still an exposed settings variable, so I think a warning can be fine, but an error seems weird, especially specifying both of them is semantically correct. Also this will not be backward compatible.

tests/test_core.py

tools/building.py

tests/test_core.py

sbc100 · 2021-03-22T23:00:27Z

Added a test for the new error/warning

tests/test_core.py

sbc100 · 2021-03-23T00:24:00Z

fake_size is not the same as disabled_size (or empty_size), its slightly bigger because the compiler driver thinks there is at least one function which can catch, so it has to enable exceptions on the JS side (it sets DISABLE_EXCEPTION_CATCHING=0).

disabled_size (or empty_size) are different because the compiler does not set DISABLE_EXCEPTION_CATCHING=0.

aheejin

shared.Settings reflects that actual current settings which is the sum of:

a) the defaults
b) any user command line settings
c) any changes made by the running of the code here (which updates a lot of settings).

In almost all cases shared.Settings is the only thing we care about because it represent the actual current setup.

settings_key_changes is for cases were we actually care about what the user specified on the commandline as opposed to (a) or (c). Its mostly used for diagnostics stuff like this. For example in this case we need to use it so we can tell the different between the default =0 and the command line =0.

I will make a couple of PRs to clean this up a bit and give settings_key_changes a better name :)

Thanks for the explanation. But sorry the same questions ;( :

@kripken said we mostly only care about command lines. Then why are other checks within this function only check shared.Settings and not settings_key_changes? Why should this one be different?
You said shared.Settings is the only thing we care about. I think that means by this point settings_key_changes has been merged into shared.Settings, right? Then why can't we check shared.Settings as the final merged info?

I'm commenting this separately because somehow I can't comment anymore in the original thread inline.

tests/test_core.py

emcc.py

aheejin · 2021-03-23T08:07:30Z

tests/test_core.py

+    err = self.expect_fail([EMCC, test_file('hello_world.c')] + self.get_emcc_args())
+    self.assertContained('error: DISABLE_EXCEPTION_CATCHING is no longer needed when specifying EXCEPTION_CATCHING_ALLOWED [-Wdeprecated] [-Werror]', err)
+
+    # =0 and =2 should be a hard error


Suggested change

# =0 and =2 should be a hard error

# =0 and =1 should be a hard error

I think having 0 as a warning is more intuitive though.

Why? The definition of =0 today (before this change) means "all functions" .. so specifying additionally an allow list makes no sense at all today.. so there should be no such users.

As of today the only value that makes sense along with the allow list is =2, right? Why should I start allowing =0 + allow list in this change? The point of this change is to try to make these two settings as mutually exclusive as possible without breaking backwards compat.

OK, I made =0 into a warning too. The difference I don't think is very important.

I think my interpretation of the switches were different. I thought DISABLE_EXCEPTION_CATCHING as a on-off switch, which we turn on whenever (in all functions or in only some of them) we want to catch exceptions, and EXCEPTION_CATCHING_ALLOWED as a fine-grained knob to say which function (if not specified, all functions).

I thought this interpretation was more intuitive, but I don't have strong opinions, so please revert if you feel like it.

sbc100 · 2021-03-23T16:48:24Z

shared.Settings reflects that actual current settings which is the sum of:
a) the defaults
b) any user command line settings
c) any changes made by the running of the code here (which updates a lot of settings).
In almost all cases shared.Settings is the only thing we care about because it represent the actual current setup.
settings_key_changes is for cases were we actually care about what the user specified on the commandline as opposed to (a) or (c). Its mostly used for diagnostics stuff like this. For example in this case we need to use it so we can tell the different between the default =0 and the command line =0.
I will make a couple of PRs to clean this up a bit and give settings_key_changes a better name :)

Thanks for the explanation. But sorry the same questions ;( :

@kripken said we mostly only care about command lines. Then why are other checks within this function only check shared.Settings and not settings_key_changes? Why should this one be different?

I think what he means is "When we want to see if a user has changed a given setting, we only need to consider about the command line". When we want to check the setting in general (regardless is seeing it was changed or not), we tend to use shared.Settings. 99.9% of settings checking is done using shared.Settings. We only use settings_key_changes when we want to be able to tell if something was set on the command line (as opposed to set in the config file). Normally we don't care about the difference, but in some cases like this one we do.

You said shared.Settings is the only thing we care about. I think that means by this point settings_key_changes has been merged into shared.Settings, right? Then why can't we check shared.Settings as the final merged info?

I think I already answered this above: We only use settings_key_changes when we want to be able to tell if something was set on the command line (as opposed to set in the config file). Normally we don't care about the difference.

I'm commenting this separately because somehow I can't comment anymore in the original thread inline.

We can instead imply this mode by the presence of the EXCEPTION_CATCHING_ALLOWED` list. This turns DISABLE_EXCEPTION_CATCHING into a binary option which is easier to deal with both internally and externally.

aheejin · 2021-03-23T20:29:22Z

We only use settings_key_changes when we want to be able to tell if something was set on the command line (as opposed to set in the config file). Normally we don't care about the difference, but in some cases like this one we do.

Can you explain a little more on what the difference between this case and other 99.9% of cases? This is mostly to understand the code myself and I don't want to block this ;) Thanks for the long explanation!

sbc100 · 2021-03-23T21:06:41Z

We only use settings_key_changes when we want to be able to tell if something was set on the command line (as opposed to set in the config file). Normally we don't care about the difference, but in some cases like this one we do.

Can you explain a little more on what the difference between this case and other 99.9% of cases? This is mostly to understand the code myself and I don't want to block this ;) Thanks for the long explanation!

Most of the time code only cares about the state of a given setting, so it quite rightly checks the Settings object

$ git grep Settings "*.py" | wc -l
799

There is also all the settings checks in JS code which I cannot easily count. (e.g. #if USE_PTHREADS etc).

Occasionally we actually care about how a settings came to be. i.e. did the user explicitly pass on the command line. In those few cases (which only exist in one place) we can use settings_key_changes, There are only nine uses of settings_key_change:

emcc.py:    strict_cmdline = settings_key_changes.get('STRICT')
emcc.py:    if settings_key_changes.get('WARN_ON_UNDEFINED_SYMBOLS') == '0':
emcc.py:    if shared.Settings.MINIMAL_RUNTIME or settings_key_changes.get('MINIMAL_RUNTIME') in ('1', '2'):
emcc.py:      if 'EXPORTED_FUNCTIONS' in settings_key_changes:
emcc.py:      if 'EXIT_RUNTIME' in settings_key_changes:
emcc.py:      if name not in settings_key_changes:
emcc.py:      if 'MODULARIZE' in settings_key_changes:
emcc.py:    if 'MEM_INIT_METHOD' in settings_key_changes:
emcc.py:      if settings_key_changes.get('WASM_BIGINT') == '0':

Most of these are cases where we want to know if the user explicitly enabled something. For example:

     if 'EXIT_RUNTIME' in settings_key_changes:                                                                      
        exit_with_error('Explictly setting EXIT_RUNTIME not compatible with STANDALONE_WASM.  EXIT_RUNTIME will always be True for programs (with a main function) and False for reactors (not main function).')

Here we have a setting called EXIT_RUNTIME that can be set on the command but is also set implicitly in STANDALONE_WASM. In this mode its not permitted to explicitly set it and the only way we can detect this is via settings_key_changes. STANDALONE_WASM is also BTW implicitly set in some cases.

sbc100 · 2021-03-23T21:08:04Z

Note that in the if 'EXIT_RUNTIME' in settings_key_changes: example I gave we actually don't care what value the setting has at all.. we only care if it was set by the user explictly.

sbc100 force-pushed the disable_exception_catching branch 3 times, most recently from 10c8e61 to eef07ba Compare March 22, 2021 19:44

sbc100 requested review from aheejin and kripken March 22, 2021 20:06

sbc100 force-pushed the disable_exception_catching branch from eef07ba to 4493480 Compare March 22, 2021 20:34

kripken reviewed Mar 22, 2021

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

emcc.py Outdated Show resolved Hide resolved

src/settings.js Outdated Show resolved Hide resolved

src/settings.js Outdated Show resolved Hide resolved

sbc100 force-pushed the disable_exception_catching branch 2 times, most recently from 04894f5 to 7dbb6c6 Compare March 22, 2021 21:12

aheejin reviewed Mar 22, 2021

View reviewed changes

sbc100 force-pushed the disable_exception_catching branch 2 times, most recently from 1bc242c to 5e57d76 Compare March 22, 2021 22:14

aheejin reviewed Mar 22, 2021

View reviewed changes

tests/test_core.py Outdated Show resolved Hide resolved

kripken approved these changes Mar 22, 2021

View reviewed changes

sbc100 force-pushed the disable_exception_catching branch 2 times, most recently from c3ce1a7 to 8e1e6f5 Compare March 22, 2021 23:00

aheejin reviewed Mar 22, 2021

View reviewed changes

tests/test_core.py Outdated Show resolved Hide resolved

tests/test_core.py Outdated Show resolved Hide resolved

sbc100 force-pushed the disable_exception_catching branch from 8e1e6f5 to fff017d Compare March 22, 2021 23:09

aheejin reviewed Mar 23, 2021

View reviewed changes

Deprecate DISABLE_EXCEPTION_CATCHING=2

d3666c0

We can instead imply this mode by the presence of the EXCEPTION_CATCHING_ALLOWED` list. This turns DISABLE_EXCEPTION_CATCHING into a binary option which is easier to deal with both internally and externally.

sbc100 force-pushed the disable_exception_catching branch from fff017d to d3666c0 Compare March 23, 2021 16:59

aheejin approved these changes Mar 23, 2021

View reviewed changes

sbc100 merged commit 85d9a28 into main Mar 23, 2021

sbc100 deleted the disable_exception_catching branch March 23, 2021 21:08

sbc100 added a commit that referenced this pull request Mar 23, 2021

Fix midair collision between #13743 and #13733

e7be594

sbc100 added a commit that referenced this pull request Mar 23, 2021

Fix midair collision between #13743 and #13733 (#13746)

7217578

	# =0 and =2 should be a hard error
	# =0 and =1 should be a hard error

Deprecate DISABLE_EXCEPTION_CATCHING=2 #13733

Deprecate DISABLE_EXCEPTION_CATCHING=2 #13733

Uh oh!

Conversation

sbc100 commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Mar 22, 2021

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Mar 23, 2021

Uh oh!

aheejin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Mar 22, 2021 •

edited

Loading

aheejin Mar 22, 2021 •

edited

Loading

sbc100 Mar 22, 2021 •

edited

Loading

aheejin left a comment •

edited

Loading

sbc100 Mar 23, 2021 •

edited

Loading

aheejin commented Mar 23, 2021 •

edited

Loading