Fix #24982: emsymbolizer failed to parse symbol map from C++ project #24994

LanderlYoung · 2025-08-21T06:46:59Z

fix symbol map line parse
unescape ascii code
update tests: test/runner "other.test_emsymbolizer*"

This PR fixes #24982

…en-core#24982 1. fix symbol map line parse 2. unescape ascii code 3. update tests: test/runner "other.test_emsymbolizer*"

test/test_symbolmap.cpp

emsymbolizer.py

dschuff

Thanks!

kripken

lgtm otherwise

ChangeLog.md

sbc100 · 2025-08-25T21:51:41Z

Is there some reason we cannot or should not just update the symbol map to avoid this mangling in the first place.

What is the source of the mangling in the first place? What style of mangling is std::out_of_range::~out_of_range\28\29 ?

dschuff · 2025-08-25T22:04:53Z

I don't know what kind of mangling that is; it doesn't exactly match others that I'm familiar with.
My assumption was that there are users that are depending on the contents of the symbol map. and would have some kind of workflow that would be broken if we were to change it. Maybe I'm wrong and we could just do that. Or, maybe it's ess of a breaking change to remove the special mangling than to go to a fully-C++-mangled state, since presumably users who depend on the current format would have their own demangling, so if we just stopped adding the mangling, maybe they wouldn't be broken?

sbc100 · 2025-08-25T22:09:08Z

I don't know what kind of mangling that is; it doesn't exactly match others that I'm familiar with. My assumption was that there are users that are depending on the contents of the symbol map. and would have some kind of workflow that would be broken if we were to change it. Maybe I'm wrong and we could just do that. Or, maybe it's ess of a breaking change to remove the special mangling than to go to a fully-C++-mangled state, since presumably users who depend on the current format would have their own demangling, so if we just stopped adding the mangling, maybe they wouldn't be broken?

I'd be tempted to make that change to the symbol map to include fully demanged symbols. @kripken do you remember why we have these strange escape codes in the map file?

kripken · 2025-08-26T00:02:38Z

I'm not sure. But isn't the symbol map just copying the name section? It should contain whatever is there iirc.

sbc100 · 2025-08-26T00:08:11Z

So maybe the issue is that the name section is broken?

sbc100 · 2025-08-26T00:14:44Z

So maybe the issue is that the name section is broken?

The name sections is fine. The problem seems to stem from the wasm-opt --print-function-map command.. I guess binaryen doesn't like those symbols.

I think we should file a binaryen bug, or maybe just avoid binaryen for this purpose (since we don't need to run process the whole wasm file, only the name section.

Co-authored-by: Alon Zakai <[email protected]>

kripken · 2025-08-26T15:42:06Z

@sbc100 do you want this PR to wait until we figure that out, or can this land?

sbc100 · 2025-08-26T15:52:44Z

@sbc100 do you want this PR to wait until we figure that out, or can this land?

I don't think we we should land this since it would only act to further lock in the strange escaping behavior

sbc100

I think we should go with #25053 instead

kripken · 2025-08-26T17:19:18Z

Oh, I think the first part of this PR is still important though:

fix symbol map line parse

That handles splitting when the line contains :: as a separator.

We can refocus this PR on just that?

sbc100 · 2025-08-26T18:42:44Z

Oh, I think the first part of this PR is still important though:

fix symbol map line parse

That handles splitting when the line contains :: as a separator.

We can refocus this PR on just that?

Ah, yes that could make sense.

LanderlYoung · 2025-08-27T03:32:16Z

Thanks guys! The name escape part has been reverted. Please review 🌹

dschuff

Looks good overall, just a couple of nits on the test.

test/test_other.py

LanderlYoung · 2025-08-28T14:20:18Z

Some other.test_codesize* tests seemed to fail, but I didn't find a relation with this code change.

dschuff

If you merge the PR to the tip of the main branch, the code size test failures should go away. And you'll have to do that anyway, since you have a conflict.

dschuff · 2025-08-28T17:47:21Z

test/test_other.py

@@ -11031,6 +11011,29 @@ def check_symbolmap_info(address, func):
    # The name section will not show bar, as it's inlined into main
    check_symbolmap_info(unreachable_addr, '__original_main')

+    # 3. Test symbol map on C++ name mangling


Suggested change

# 3. Test symbol map on C++ name mangling

# 3. Test symbol map on demangled C++ names

sbc100 · 2025-08-28T18:44:27Z

emsymbolizer.py

@@ -218,12 +218,19 @@ def symbolize_address_sourcemap(module, address, force_file):

 def symbolize_address_symbolmap(module, address, symbol_map_file):
  """Symbolize using a symbol map file."""
+
+  def split_symbolmap_line(line):
+      index = line.find(':')


I think you can just do return line.split(':', 1)

If you want and explicit error you could also add assert ':' in line, 'f'invalid symbolmap line: {line}'

sbc100 · 2025-08-28T18:51:53Z

test/test_other.py

+    check_cpp_symbolmap_info(unreachable_addr, 'Namespace::bar')            # the function name
+    check_cpp_symbolmap_info(unreachable_addr, 'Namespace::ClassA')         # the type parameter
+    check_cpp_symbolmap_info(unreachable_addr, 'Namespace::ClassB')         # the parameter
+    check_symbol_map_contains('out_to_js')                                  # the imported function


Actually I think maybe we should be making test_symbol_map any more complex by including emsymbolizer stuff too. Can we go back adding this to a new or existing test_emsymboliser_xx test?

Or... could we just turn test_dwarf.c into a c++ test and then have the existing symbol map test include C++ symbols? that would make this a very small increment over the existing test.

LanderlYoung force-pushed the bufix/emsymbolizer_symbolmap branch from c81e65c to d93eb85 Compare August 21, 2025 07:24

fix emsymbolizer failed to parse symbol map from C++ project emscript…

f20fd5e

…en-core#24982 1. fix symbol map line parse 2. unescape ascii code 3. update tests: test/runner "other.test_emsymbolizer*"

LanderlYoung force-pushed the bufix/emsymbolizer_symbolmap branch from d93eb85 to f20fd5e Compare August 21, 2025 08:53

LanderlYoung added 2 commits August 21, 2025 19:16

revert test changes

3e5a57a

add c++ symbol map test

d792fa6

LanderlYoung force-pushed the bufix/emsymbolizer_symbolmap branch from 549ccad to d792fa6 Compare August 21, 2025 11:41

LanderlYoung changed the title ~~fix #24982: emsymbolizer failed to parse symbol map from C++ project~~ Fix #24982: emsymbolizer failed to parse symbol map from C++ project Aug 21, 2025

kripken reviewed Aug 21, 2025

View reviewed changes

test/test_symbolmap.cpp Outdated Show resolved Hide resolved

emsymbolizer.py Outdated Show resolved Hide resolved

LanderlYoung and others added 3 commits August 22, 2025 12:07

fix cr comments

67d965d

fix cr comments

aebc4ca

Merge branch 'main' into bufix/emsymbolizer_symbolmap

756d1d1

dschuff approved these changes Aug 22, 2025

View reviewed changes

kripken approved these changes Aug 22, 2025

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

LanderlYoung and others added 3 commits August 26, 2025 15:35

Update ChangeLog.md according to CR

bb45a47

Co-authored-by: Alon Zakai <[email protected]>

Merge branch 'main' into bufix/emsymbolizer_symbolmap

826469e

Merge branch 'main' into bufix/emsymbolizer_symbolmap

113b1aa

sbc100 requested changes Aug 26, 2025

View reviewed changes

LanderlYoung requested a review from sbc100 August 27, 2025 03:17

revert symbol name escape, keep only the symbolmap parse logic

b881a2e

LanderlYoung force-pushed the bufix/emsymbolizer_symbolmap branch from 3a59a68 to b881a2e Compare August 27, 2025 03:30

dschuff reviewed Aug 27, 2025

View reviewed changes

test/test_other.py Outdated Show resolved Hide resolved

test/test_other.py Show resolved Hide resolved

test/test_other.py Show resolved Hide resolved

fix cr comments

b2ffeb2

LanderlYoung force-pushed the bufix/emsymbolizer_symbolmap branch from af71e60 to b2ffeb2 Compare August 28, 2025 06:32

LanderlYoung added 2 commits August 28, 2025 14:38

add out_to_js check

3e69fdb

add comment to test_symbol_map.cpp

39f3964

dschuff reviewed Aug 28, 2025

View reviewed changes

sbc100 reviewed Aug 28, 2025

View reviewed changes

	# 3. Test symbol map on C++ name mangling
	# 3. Test symbol map on demangled C++ names

Fix #24982: emsymbolizer failed to parse symbol map from C++ project #24994

Are you sure you want to change the base?

Fix #24982: emsymbolizer failed to parse symbol map from C++ project #24994

Uh oh!

Conversation

LanderlYoung commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dschuff left a comment

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Aug 25, 2025

Uh oh!

dschuff commented Aug 25, 2025

Uh oh!

sbc100 commented Aug 25, 2025

Uh oh!

kripken commented Aug 26, 2025

Uh oh!

sbc100 commented Aug 26, 2025

Uh oh!

sbc100 commented Aug 26, 2025

Uh oh!

kripken commented Aug 26, 2025

Uh oh!

sbc100 commented Aug 26, 2025

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

kripken commented Aug 26, 2025

Uh oh!

sbc100 commented Aug 26, 2025

Uh oh!

LanderlYoung commented Aug 27, 2025

Uh oh!

dschuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LanderlYoung commented Aug 28, 2025

Uh oh!

dschuff left a comment

Choose a reason for hiding this comment

Uh oh!

dschuff Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

dschuff Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LanderlYoung commented Aug 21, 2025 •

edited

Loading