Skip to content

Conversation

@kleisauke
Copy link
Collaborator

toString() is undeclared.

Fixes a regression introduced in commit 04a0cad.

`toString()` is undeclared.

Fixes a regression introduced in commit 04a0cad.
@kleisauke
Copy link
Collaborator Author

Details:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emscripten_temp_50snxhja/vips-es6.jso3.js:757:85: ERROR - [JSC_UNDEFINED_VARIABLE] variable toString is undeclared
  757|     out("aligned_realloc(ptr=" + ptrToString($0) + ", alignment=" + $1 + ", size=" + toString(Number($2)));
                                                                                            ^^^^^^^^

1 error(s), 0 warning(s)

@kripken kripken requested a review from sbc100 December 4, 2024 17:35
@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2024

Are we missing a test for -sMALLOC=emmalloc-verbose?

@kripken
Copy link
Member

kripken commented Dec 4, 2024

Yes, looks like that isn't covered by any existing test. Might be worth adding, but it is a setting for internal debugging.

@kleisauke
Copy link
Collaborator Author

It looks like this is being covered here:

'memvalidate_verbose': ['-DEMMALLOC_MEMVALIDATE', '-DEMMALLOC_VERBOSE', '-DRANDOM_ITERS=130'],

'memvalidate_verbose': (['-sMALLOC=emmalloc-memvalidate-verbose'],),

However, these tests do not verify the debug prints. I'm not sure if that would be useful.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 5, 2024

It looks like this is being covered here:

'memvalidate_verbose': ['-DEMMALLOC_MEMVALIDATE', '-DEMMALLOC_VERBOSE', '-DRANDOM_ITERS=130'],

'memvalidate_verbose': (['-sMALLOC=emmalloc-memvalidate-verbose'],),

However, these tests do not verify the debug prints. I'm not sure if that would be useful.

But you could add --closure=1 which would cause the current code to fail right?

@kleisauke
Copy link
Collaborator Author

But you could add --closure=1 which would cause the current code to fail right?

Ah, good point. Addressed with commit 6fef505, PTAL.

})
def test_emmalloc(self, *args):
if '-DEMMALLOC_VERBOSE' in args and self.is_wasm64():
self.skipTest('EMMALLOC_VERBOSE is not compatible with wasm64')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice to see this skip being removed!

if '-DEMMALLOC_VERBOSE' in args and self.is_wasm64():
self.skipTest('EMMALLOC_VERBOSE is not compatible with wasm64')
self.maybe_closure()
self.set_setting('DEFAULT_LIBRARY_FUNCS_TO_INCLUDE', ['$ptrToString'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this line should be needed.

Can we instead add EM_JS_DEPS(deps, "$ptrToString"); to emmalloc.c?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required by `emmalloc_{dump,validate}_memory_regions()`.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm and that test failure on CI looks unrelated.

@kripken kripken merged commit 98f59c7 into emscripten-core:main Dec 10, 2024
27 of 28 checks passed
@kleisauke kleisauke deleted the emmalloc-fix-verbose branch December 11, 2024 09:04
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
toString() was undeclared.

Fixes a regression introduced in commit 04a0cad.
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.

3 participants