Skip to content

Conversation

@fs-eire
Copy link
Contributor

@fs-eire fs-eire commented Feb 14, 2025

Fixes #23672

@fs-eire
Copy link
Contributor Author

fs-eire commented Feb 14, 2025

@sbc100 Could you help me create a test case?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 14, 2025

I suggest adding a new test to test_other.py see all the existing tests that start with def test_jslib_* in that file.

For example:

$ ./test/runner other.test_jslib_native_deps
Test suites:
['test_other']
Running test_other: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_jslib_native_deps (test_other.other.test_jslib_native_deps) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.714s

OK

@fs-eire
Copy link
Contributor Author

fs-eire commented Feb 14, 2025

could you suggest a name for the test case so that it matches the style and naming convention best?

EDIT: I would like to add 2 test cases, one for successful run and one for failure because of non-empty.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 14, 2025

How about test_jslib_new_objects?

@fs-eire
Copy link
Contributor Author

fs-eire commented Feb 15, 2025

I am working on adding a test and I found this strange behavior:

  def test_jslib_new_objects_basic(self):
    create_file('lib.js', '''
      addToLibrary({
        $obj: {
          a: new Map(),
          b: new Set(),
          c: new WeakMap(),
          d: new WeakSet(),
        }
      });
      ''')
    self.run_process([EMCC, test_file('hello_world.c'), '--js-library=lib.js', '-sEXPORTED_FUNCTIONS=obj,_main'])

I have this line in function stringifyWithFunctions(obj):

  console.log(`##Debug ${inspect(obj)}, ${obj instanceof Set}, ${obj.size}`);

And I get the following outputs:

##Debug {
  a: Map(0) {},
  b: Set(0) {},
  c: WeakMap { <items unknown> },
  d: WeakSet { <items unknown> }
}, false, undefined
##Debug Map(0) {}, false, 0
##Debug Set(0) {}, false, 0
##Debug WeakMap { <items unknown> }, false, undefined
##Debug WeakSet { <items unknown> }, false, undefined

@sbc100 Did you have any tricks in jsifier.mjs? I cannot explain b instanceof Set === false.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 15, 2025

That could be related to the fact that we evaluate the library code in a separate JS context? See compileTimeContext = vm.createContext in utility.mjs

@sbc100
Copy link
Collaborator

sbc100 commented Feb 15, 2025

From reading https://nodejs.org/api/vm.html#what-does-it-mean-to-contextify-an-object it sounds like you may be able to add vm.constants.DONT_CONTEXTIFY to the vm.createContext call and then use instanceof compileTimeContext.Set?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 18, 2025

Thanks for working on this!

@fs-eire
Copy link
Contributor Author

fs-eire commented Feb 18, 2025

The following test failure:

file:///home/circleci/project/src/utility.mjs:295
export const compileTimeContext = vm.createContext(vm.constants.DONT_CONTEXTIFY);
                                                                ^

TypeError: Cannot read properties of undefined (reading 'DONT_CONTEXTIFY')

From API history of vm.runInNewContext

v22.8.0, v20.18.0 | The contextObject argument now accepts vm.constants.DONT_CONTEXTIFY.

Does emscripten uses lower version of this?

@fs-eire
Copy link
Contributor Author

fs-eire commented Feb 18, 2025

@sbc100 do you want to update minimum node version requirement or revert this change to use vm.runInContext('[Map, Set, WeakMap, WeakSet]'?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 18, 2025

@sbc100 do you want to update minimum node version requirement or revert this change to use vm.runInContext('[Map, Set, WeakMap, WeakSet]'?

The later please. Sorry for the back a forth on this :(

@fs-eire fs-eire force-pushed the jsifier-stringifyWithFunctions-support-set-map branch from 51f0127 to ebcb1ca Compare February 18, 2025 21:14
@sbc100 sbc100 changed the title support ES6 Set/Map in stringifyWithFunctions Support ES6 Set/Map in stringifyWithFunctions Feb 18, 2025
@sbc100 sbc100 enabled auto-merge (squash) February 18, 2025 22:53
@sbc100 sbc100 merged commit 5dfe0a2 into emscripten-core:main Feb 19, 2025
29 checks passed
fs-eire added a commit to microsoft/onnxruntime that referenced this pull request Feb 27, 2025
### Description

Upgrade EMSDK to 4.0.4


### Motivation and Context

Emscripten v4.0.4 brings 2 useful changes that are helpful for webgpu:

- emscripten-core/emscripten#23678
- emscripten-core/emscripten#23631
guschmue pushed a commit to microsoft/onnxruntime that referenced this pull request Mar 6, 2025
### Description

Upgrade EMSDK to 4.0.4


### Motivation and Context

Emscripten v4.0.4 brings 2 useful changes that are helpful for webgpu:

- emscripten-core/emscripten#23678
- emscripten-core/emscripten#23631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using Map in library code seems not allowed

2 participants