emscripten_strict #4665

juj · 2016-10-24T22:26:49Z

This adds a new EMSCRIPTEN_STRICT build mode, enabled either with -s EMSCRIPTEN_STRICT=1 or with env. var EMSCRIPTEN_STRICT=1. That does the following things:

The system-provided #define EMSCRIPTEN preprocessor value disappears, but code should use #ifdef __EMSCRIPTEN__.
The linker flag -s ERROR_ON_UNDEFINED_SYMBOLS defaults to 1 instead of the previous default 0. (Default to erroring out link stage if there are any remaining undefined symbols. #2714)
Adds a new linker flag -s ERROR_ON_MISSING_LIBRARIES=0/1, which defaults to 0, but in EMSCRIPTEN_STRICT mode defaults to 1. (Passing a nonexistent -llibthatisnotthere should be (configurable to be) an error #4213)
Removes system/include/emscripten from the set of system include directories in EMSCRIPTEN_STRICT mode, with the intent that users should do #include <emscripten/foo.h> to include Emscripten-related system headers (see also include_emscripten #4664)
Stops autolinking to most src/library_xx.js files, but instead code should use one of the predefined library aliases to link to system libraries, e.g. -lGL for OpenGL or -lSDL for SDL 1.x etc., or by using -lfoo.js to link to them. (Add support for conditionally excluding all handwritten JS code for SDL. #2730)

The intent is that these all would become deprecated options, and people would be able to use EMSCRIPTEN_STRICT mode to try out ahead of time that their projects build without relying on deprecated features.

In this PR, building without EMSCRIPTEN_STRICT mode doesn't yet change any existing behavior, but all changes are opt in at this point (although with the wording being about the intent to deprecate)

Tried to get as much of the breaking changes in one go to highlight/group the different things that I'd like to change. Note that these are all more or less validation related, where it would be nicest to be as strict as possible by default, like native compilers are.

juj · 2016-10-24T22:31:00Z

Need to run full suite before merging, but wanted to post this already for initial comments. Also needs a good heads up conversation on the mailing list, but let's first discuss here that the different deprecations are something that we want to agree on, and what would be the best way to bring those in. (In one go like this, or separately?)

kripken · 2016-10-24T22:53:25Z

Overall this sounds good. Main concerns are, as you said, to not surprise people and discuss everything well in advance, and yeah, a mailing list post to point to here makes sense.

This adds a new EMSCRIPTEN_STRICT build mode, enabled either with -s EMSCRIPTEN_STRICT=1 or with env. var EMSCRIPTEN_STRICT=1.

How about -s STRICT=1 for the option (seems odd to have EMSCRIPTEN_ in an option), and EMCC_STRICT=1 for the env var (others start with EMCC)?

juj · 2016-10-24T22:57:12Z

Alright. In the first version/sdk release, let's land this in a manner that does not remove any features, but just adds the strict mode. And in a later release, do the actual removal. That would give the kind of "two stage deprecate->remove" process to ensure not too abrupt changes. I'll post to the mailing list about this one to raise awareness.

kripken · 2016-10-24T22:59:19Z

emcc.py

+    if not use_js:
+      cmd += shared.EMSDK_OPTS + ['-D__EMSCRIPTEN__']
+      # The preprocessor define EMSCRIPTEN is deprecated. Don't pass it to code in strict mode. Code should use the define __EMSCRIPTEN__ instead.
+      if not os.environ.get('EMSCRIPTEN_STRICT') or int(os.environ.get('EMSCRIPTEN_STRICT')) == 0: cmd += ['-DEMSCRIPTEN']


would be nicer to have a variable strict_mode or such, that does the environment check once at the top of the file.

in fact maybe it could be in tools/shared.py, as others might need it?

Sounds good.

kripken · 2016-10-24T23:01:01Z

tools/shared.py

+  COMPILER_OPTS = COMPILER_OPTS + ['-D__EMSCRIPTEN__']
+
+  # The preprocessor define EMSCRIPTEN is deprecated. Don't pass it to code in strict mode. Code should use the define __EMSCRIPTEN__ instead.
+  if not os.environ.get('EMSCRIPTEN_STRICT') or int(os.environ.get('EMSCRIPTEN_STRICT')) == 0: COMPILER_OPTS += ['-DEMSCRIPTEN']


ah yes, it is needed here. so maybe have Building.is_strict(), in this file, and we call it from all the places that need to ask if we are in strict mode?

Added shared.is_emscripten_strict(). Could not put it in Building, because that is too late in the file for early bits of the Settings-related code to refer to it (and reordering would create other kind of dependency issue, with Building referring to Settings too early), so made it a self-standing function at the top of the file.

kripken · 2016-10-24T23:02:08Z

Meanwhile reading this a second time, I do see the logic in calling the option EMSCRIPTEN_STRICT - it's something about the behavior of emscripten itself, specifically. So I could go either way. Let's see what more people think.

dschuff · 2016-10-25T16:24:27Z

+1, this is great; it should result in more consistent (and therefore hopefully less surprising) behavior, and allow us more implementation flexibility.
I think just STRICT is fine for the -s flag, and whatever is consistent with other env vars for that.

juj · 2016-10-25T17:01:34Z

I think it helps to have the linker flag -s EMSCRIPTEN_STRICT=1 coincide with the env. var name export EMSCRIPTEN_STRICT=1 so that one wouldn't need to rememeber two separate names for the same feature?

kripken · 2016-10-25T17:23:05Z

Well, this would be the first env var that has the exact same name as a setting - which opens up opportunities for confusion. Actually, though, why do we need both an env var and a setting here?

juj · 2016-10-25T17:41:47Z

Well, this would be the first env var that has the exact same name as a setting - which opens up opportunities for confusion. Actually, though, why do we need both an env var and a setting here?

I'd like to give as much flexibility as possible to be able to easily configure the feature to builds. Env. vars are great for locally/temporarily setting a value to try out once, but linker flags are better for build systems.

kripken · 2016-10-25T18:16:49Z

Are you suggesting we temporarily support the two ways, or forever?

juj · 2016-10-26T09:23:45Z

Ok, renamed to env. var EMCC_STRICT=1 and -s STRICT=1. I'm suggesting that we do support this env. var and linker flag permanently, but that the features that they add are not permanent. That is, the changes that strict mode adds now, e.g. no #define EMSCRIPTEN etc. would not be fixed to be things only in strict mode, but in the future what is now strict mode would be the default mode, so in a sense use strict mode as a "landing pad" to introduce deprecations (when there is no other flag to do so).

kripken · 2016-10-26T17:27:28Z

emcc.py

-          logging.warning('emcc: cannot find library "%s"', lib)
+        if not found:
+          # Some native libraries are implemented in Emscripten as system side JS libraries
+          js_system_libraries = {


how about refactoring this js system lib logic into Building in tools/shared.py?

The code is dependant on settings_changes before applying them, since it's filtering out which command line link items to accept/fail on. I'm not sure why it would be useful to separate it out?

I agree it's not code likely to be used elsewhere. But just imagine adding a js library in the future, it feels nicer to edit a small function in shared.py rather than some code in the middle of emcc.py?

Ok, moved the code to Building.

kripken · 2016-10-26T17:29:54Z

tools/shared.py

@@ -931,8 +935,10 @@ def get_llvm_target():

 if LLVM_TARGET == WASM_TARGET:
  # wasm target does not automatically define emscripten stuff, so do it here.
-  COMPILER_OPTS = COMPILER_OPTS + ['-DEMSCRIPTEN',
-                                   '-D__EMSCRIPTEN__']
+  COMPILER_OPTS = COMPILER_OPTS + ['-D__EMSCRIPTEN__']


looks like this setting of the emscripten define is duplicated in emcc.py? if we need it in both (but do we?), perhaps we could refactor it into a shared method?

I'm not sure, the machinery touches the old EMCONFIGURE_JS hacks that are used to support the ./configure architecture. Looks like that mode does not have __EMSCRIPTEN__ set, as it shouldn't(?).

Yes, I see, the hacks there need to only add it in some cases.

I suppose we could have a add_emscripten_define method somewhere, which returns the one define or both depending on if we are in strict mode. How does that sound?

I'm not sure if adding that is much worth it, since this occurs exactly in two places, and it's quite lean to just add it in these locations. Eventually we'll be tearing down the code anyways when we get to remove #define EMSCRIPTEN altogether, so building a new abstraction for this seems temporary? Left this one as is for now, let me know if you feel strongly about this.

kripken · 2016-10-26T17:32:11Z

tools/shared.py

@@ -471,6 +471,10 @@ def find_temp_directory():
 def get_emscripten_version(path):
  return open(path).read().strip().replace('"', '')

+# Returns true if Emscripten is running in 'strict' mode, in which deprecated compiler features are not supported.
+def is_emscripten_strict():
+  return (os.environ.get('EMCC_STRICT') and int(os.environ.get('EMCC_STRICT')) != 0) or Settings.STRICT


this can try to access Settings.STRICT before the Settings object exists, that worries me a lot.

In particular, we should have tests that check usage with both the env var and without, of things both working and not, so we hit that code path.

Hit that case, and had to adapt.

juj · 2016-10-26T18:53:32Z

Pushed some new changes, namely made the browser suite to pass fully when running in EMCC_STRICT mode enabled. @kripken: could you take a fresh peek?

juj · 2016-11-19T18:23:39Z

Updated the PR and rebased to latest. This should be good to go now.

kripken · 2016-11-21T18:50:25Z

emcc.py

+        if not found:
+          system_js_libraries += shared.Building.path_to_system_js_libraries(lib)
+
+      # Certain linker flags imply some link libraries to be pulled in by default.


i still feel these lines are nicer in something like shared.Building.add_default_system_libraries_from_settings_changes (or a better name ;) ), but i don't feel strongly about it.

Maybe leave that for a possible followup.

Sure, added a function for that in Building. Not sure if I managed a better name though :)

kripken · 2016-11-21T18:51:20Z

src/library_fs.js

-  $FS__deps: ['$ERRNO_CODES', '$ERRNO_MESSAGES', '__setErrNo', '$PATH', '$TTY', '$MEMFS', '$IDBFS', '$NODEFS', '$WORKERFS', 'stdin', 'stdout', 'stderr'],
+  $FS__deps: ['$ERRNO_CODES', '$ERRNO_MESSAGES', '__setErrNo', '$PATH', '$TTY', '$MEMFS',
+#if __EMSCRIPTEN_HAS_idbfs_js__
+  '$IDBFS',


indentation not consistent with the following ones

kripken · 2016-11-21T18:54:29Z

src/settings.js

+                                    // building with -s ERROR_ON_MISSING_LIBRARIES=0 setting,
+                                    // prefer to set that option explicitly in your build system.
+
+var SYSTEM_JS_LIBRARIES = []; // Specifies a list of Emscripten-provided JS libraries to link against.


Currently we have js_libraries in emcc.py, which invokes emscripten.py with --libraries. It seems like emcc.py could use this new variable, and we could get rid of --libraries? That would avoid duplicated functionality.

Although for people using emscripten.py directly, maybe we shouldn't remove that.

Hmm, that might be true, though not sure if I want to change this right now at least, there doesn't seem to be any tests for the --libraries flag in our test suite.

kripken · 2016-11-21T18:56:54Z

tools/shared.py

@@ -472,6 +472,15 @@ def find_temp_directory():
 def get_emscripten_version(path):
  return open(path).read().strip().replace('"', '')

+# Returns true if Emscripten is running in 'strict' mode, in which deprecated compiler features are not supported.
+def is_emscripten_strict():
+  if os.environ.get('EMCC_STRICT') and int(os.environ.get('EMCC_STRICT')) != 0: return True


this int operation may throw, if the value is e.g. a. instead, please do os.environ.get('EMCC_STRICT') != '0'

I think it's good to throw, no? That would indicate someone messed up setting the env. var, so a critical abort there would make sense, as opposed to e.g. silently treating export EMCC_STRICT=a as falsey or something similar.

throwing might be ok, but it'll throw something like ValueError: invalid literal for int() with base 10 which is not very clear. and in all other places, we accept '0' as "no", and everything else, including "a" as yes.

kripken · 2016-11-21T18:58:09Z

tools/shared.py

                                   '-Dunix',
                                   '-D__unix',
                                   '-D__unix__']

+  # The preprocessor define EMSCRIPTEN is deprecated. Don't pass it to code in strict mode. Code should use the define __EMSCRIPTEN__ instead.
+  if not is_emscripten_strict(): COMPILER_OPTS += ['-DEMSCRIPTEN']


i think we discussed this already - this code happens before the STRICT setting is applied. so only the env var can influence this I think? ditto below.

Yeah, that's right. Revised the code flow for this now.

kripken · 2016-11-21T19:00:00Z

Some testing comments:

I see you got the test suite to pass with or without STRICT? Is there a test that verifies the old behavior, e.g. a GL test that doesn't -lGL?
We need direct tests for the strict mode features, checking both the env and the setting, in test_other.py.

juj · 2016-11-22T03:18:32Z

I see you got the test suite to pass with or without STRICT? Is there a test that verifies the old behavior, e.g. a GL test that doesn't -lGL?

Added test browser.test_sdl1_in_emscripten_nonstrict_mode to verify that code still passes with the old behavior.

We need direct tests for the strict mode features, checking both the env and the setting, in test_other.py.

Added test other.test_error_on_missing_libraries to verify that the strict mode applies correctly on both command line and env. var, and that the defaults for strict mode for this specific flag are as expected.

juj · 2016-11-22T03:21:57Z

When testing, I realize that the preprocessor define check goes all the way to emscripten-core/emscripten-fastcomp-clang#11

kripken · 2016-11-22T18:38:45Z

Thanks, lgtm aside from that tiny test thing.

kripken · 2016-11-22T18:55:14Z

Oh and the '0' thing.

… libraries, but instead require explicitly specifying which ones to link to on command line.

…nker flag.

…ripten.h> and #include <emscripten/emscripten.h> can be used in the future.

…uite to pass in EMCC_STRICT mode.

…ding in tools/shared.py.

… code path in shared.Building.

…ys visible.

…ies.

juj · 2016-12-07T06:16:22Z

Changed the '0' thing and rebased to latest. This would be good to merge in before merge to master. Existing behavior should not be changed in any way, so should be safe to add in.

juj · 2016-12-16T13:47:54Z

@kripken: Does this look good to you to merge? I'd really like to get this in to 1.37.0, because this adds the new -s ERROR_ON_MISSING_LIBRARIES=0/1 option so people will have an ability to turn that one on already.

kripken · 2016-12-16T18:09:26Z

emcc.py

@@ -918,6 +922,24 @@ def detect_fixed_language_mode(args):
      if separate_asm:
        shared.Settings.SEPARATE_ASM = os.path.basename(asm_target)

+      if 'EMCC_STRICT' in os.environ:


this happens after the code above for -DEMSCRIPTEN, so it seems it would only work for the setting and not the environment var?

or wait, is the above for the configure stuff? then it's ok.

kripken · 2016-12-16T18:10:34Z

emcc.py

+      if 'EMCC_STRICT' in os.environ:
+        shared.Settings.STRICT = os.environ.get('EMCC_STRICT') != '0'
+
+      STRICT = ([None] + filter(lambda x: x.startswith('STRICT='), settings_changes))[-1]


why [None]?

Hmm I guess it doesn't matter much, let me go with False instead since it's evaluated to true/false below.

Sorry, I wasn't clear. My issue isn't with None vs False. It's that I don't understand that line - why does it need [..something..] at all? I guess it's so the -1 can work, because we happen to just want the last one but the line just seems overcomplicated.

and we do this twice. how about adding a method, get_last_change which returns the last change, or None if there isn't one?

Ohh, I see. Refactored that to its own function, that looks better now, what do you think?

kripken · 2016-12-16T18:11:26Z

emcc.py

+        shared.Settings.STRICT = os.environ.get('EMCC_STRICT') != '0'
+
+      STRICT = ([None] + filter(lambda x: x.startswith('STRICT='), settings_changes))[-1]
+      if STRICT:


I worry about creating a var called STRICT, might confuse us in the future as to what to use. How about temp_strict or strict_changes?

Ok, opting for strict_cmdline since the variable comes from a command line setting.

kripken · 2016-12-16T18:12:23Z

emcc.py

+
+      STRICT = ([None] + filter(lambda x: x.startswith('STRICT='), settings_changes))[-1]
+      if STRICT:
+        shared.Settings.STRICT = int(STRICT[len('STRICT='):])


The settings changes would set Settings.STRICT for you - why do we even need to do this?

Oh I see, because we need to do the library stuff first.

But then, let's comment that above here and not just below.

kripken · 2016-12-16T18:14:24Z

emcc.py

+        shared.Settings.ERROR_ON_MISSING_LIBRARIES = 1
+
+      # Libraries are searched before settings_changes are applied, so pull its value from the command line already here.
+      ERROR_ON_MISSING_LIBRARIES = ([None] + filter(lambda x: x.startswith('ERROR_ON_MISSING_LIBRARIES='), settings_changes))[-1]


also, why [None]?

Changed to False as well.

kripken · 2016-12-18T22:16:32Z

emcc.py

+      # Libraries are searched before settings_changes are applied, so apply the value for STRICT and ERROR_ON_MISSING_LIBRARIES from
+      # command line already now.
+
+      def last_element_or_none(arr):


thanks, but let's share even more code here. this is only used on the settings_changes array. so you can call this get_last_settings_change, and receive the name of the pref as a param

lgtm with that. might be some more cleanup worth doing, but we can do it in followups.

Ahh ok, updated.

…emcc --clear-ports would fail.

kripken reviewed Oct 24, 2016

View reviewed changes

juj force-pushed the emscripten_strict branch from 7665d8b to 8f9f7e9 Compare October 26, 2016 08:55

kripken reviewed Oct 26, 2016

View reviewed changes

juj force-pushed the emscripten_strict branch from b71f229 to 862c9aa Compare October 26, 2016 18:51

juj force-pushed the emscripten_strict branch from 862c9aa to 70f2b71 Compare November 19, 2016 18:03

kripken reviewed Nov 21, 2016

View reviewed changes

juj added 10 commits December 7, 2016 03:01

In EMSCRIPTEN_STRICT mode, stop autolinking to all system provided JS…

1b3fe42

… libraries, but instead require explicitly specifying which ones to link to on command line.

Rename Emscripten strict mode to EMCC_STRICT=1 env var/-s STRICT=1 li…

72c97d1

…nker flag.

Add root level include emscripten.h so that both forms #include <emsc…

381eca8

…ripten.h> and #include <emscripten/emscripten.h> can be used in the future.

Finalize EMCC_STRICT mode for linking to libraries. Improve browser s…

af265a5

…uite to pass in EMCC_STRICT mode.

Move the deduction of which system JS libraries to link to class Buil…

1b4e784

…ding in tools/shared.py.

Apply the set of system JS libraries based on the link settings via a…

11e4634

… code path in shared.Building.

Indentation fix in src/library_fs.js

12d47b0

Revise how EMCC_STRICT and -s STRICT=1 are read so that they are alwa…

815a35b

…ys visible.

Add test for scenario when not explicitly linking to system JS librar…

022c4db

…ies.

Accept EMCC_STRICT=somethingotherthanzero as true for EMCC_STRICT

08b70e5

juj force-pushed the emscripten_strict branch from 13ccbfe to 08b70e5 Compare December 7, 2016 01:02

juj added 2 commits December 7, 2016 08:12

Add export of JS lib ifdefs for all libs, even when not in strict mode.

267c4ee

Fix typo in looking up path_from_root()

8455fba

gouletr mentioned this pull request Dec 12, 2016

stricter_gl_extension_headers #4502

Closed

juj mentioned this pull request Dec 16, 2016

1.37.0 merge to master #4803

Closed

kripken reviewed Dec 16, 2016

View reviewed changes

juj force-pushed the emscripten_strict branch from 3d39bf5 to 4ec5caa Compare December 18, 2016 11:09

kripken reviewed Dec 18, 2016

View reviewed changes

Apply code review to Emscripten strict mode.

9b727e8

juj force-pushed the emscripten_strict branch from 4ec5caa to 9b727e8 Compare December 19, 2016 08:27

juj merged commit 051ef31 into emscripten-core:incoming Dec 19, 2016

juj added a commit that referenced this pull request Dec 19, 2016

Fix bug after PR #4665 that rebuilding SDL2 with -s USE_SDL2=1 after …

485a49b

…emcc --clear-ports would fail.

juj mentioned this pull request Jul 10, 2017

Significant overhaul of the OpenAL implementation #5367

Merged

Wenzhao-Xiang mentioned this pull request Aug 29, 2019

[GSoC 2019] Improve the performance of JavaScript version of OpenCV (OpenCV.js) opencv/opencv#15371

Merged

emscripten_strict #4665

emscripten_strict #4665

Conversation

juj commented Oct 24, 2016 • edited Loading

juj commented Oct 24, 2016

kripken commented Oct 24, 2016 • edited Loading

juj commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Oct 24, 2016

dschuff commented Oct 25, 2016

juj commented Oct 25, 2016

kripken commented Oct 25, 2016

juj commented Oct 25, 2016

kripken commented Oct 25, 2016

juj commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juj commented Oct 26, 2016

juj commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Nov 21, 2016

juj commented Nov 22, 2016

juj commented Nov 22, 2016

kripken commented Nov 22, 2016

kripken commented Nov 22, 2016

juj commented Dec 7, 2016 • edited Loading

juj commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juj commented Oct 24, 2016 •

edited

Loading

kripken commented Oct 24, 2016 •

edited

Loading

juj commented Dec 7, 2016 •

edited

Loading