Fixes regarding explicit names #6466

vouillon · 2024-04-03T09:19:34Z

only write explicit function names
when merging modules, the name of types, globals and tags in all modules but the first were lost

src/ir/module-splitting.cpp

vouillon · 2024-04-03T09:23:33Z

src/wasm/wasm-binary.cpp

-      o << U32LEB(emitted);
-      writeEscapedName(curr->name.str);
-      emitted++;
+    std::vector<std::pair<Index, Function*>> functionsWithNames;


This is the very same code as for globals.

- only write explicit function names - when merging modules, the name of types, globals and tags in all modules but the first were lost

test/unit/input/gc_target_feature.wasm

src/ir/module-splitting.cpp

kripken · 2024-04-03T23:20:26Z

src/ir/module-utils.cpp

@@ -48,6 +48,7 @@ Function* copyFunction(Function* func,
                       std::optional<std::vector<Index>> fileIndexMap) {
  auto ret = std::make_unique<Function>();
  ret->name = newName.is() ? newName : func->name;
+  ret->hasExplicitName = func->hasExplicitName && !newName.is();


Why not set it to true when newName.is()?

newName is a generated name. For instance, some names like outlined-A$func-name are generated during inlining. But maybe we can consider it is explicit enough?

Ah, I see what you mean, good point. I guess it depends on the user expectation in this code. I think that we do want to keep such names, as we are copying a function, so we should copy the property of having an explicit name.

kripken · 2024-04-05T17:56:30Z

src/ir/module-utils.cpp

@@ -48,6 +48,7 @@ Function* copyFunction(Function* func,
                       std::optional<std::vector<Index>> fileIndexMap) {
  auto ret = std::make_unique<Function>();
  ret->name = newName.is() ? newName : func->name;
+  ret->hasExplicitName = newName.is() || func->hasExplicitName;


Suggested change

ret->hasExplicitName = newName.is() || func->hasExplicitName;

ret->hasExplicitName = func->hasExplicitName;

I think it's better to just match the copied function. Their names may differ (if copied into the same module) but the property of having an explicit name seems like it should be the same, at least by default.

Oh, indeed!

kripken · 2024-04-05T17:59:13Z

src/ir/module-utils.cpp

@@ -222,7 +231,6 @@ void copyModule(const Module& in, Module& out) {
  out.customSections = in.customSections;
  out.debugInfoFileNames = in.debugInfoFileNames;
  out.features = in.features;
-  out.typeNames = in.typeNames;


Why did this move? The description of the function it was moved to is that it copies toplevel module items like Functions and Globals, and not metadata.

So, you think I should rather put the code that copy type names in wasm-merge.cc?

Sorry, I don't see why my question, which was about the deletion of this line, is answered by something about wasm-merge? What is the context I am missing?

From the standpoint of this file by itself (not thinking about wasm-merge at all), this change seems surprising as I said - it looks like this code moved up to copyModuleItems which I don't understand the motivation for.

wasm-merge currently does not copy type names from the input modules to the merged module. I added some code for that in copyModuleItems. But then this line became unnecessary.

I see, so wasm-merge uses copyModuleItems and not copyModule. The difference between those two decreases with this change, and the meaning of the difference becomes less clear to me. Perhaps we don't need two functions here at all, and there could just be a single function with maybe a set of options as to whether to copy exports, the start method, etc. (the things only in copyModule). Anyhow, for this PR I think the change you made is good, but please add a TODO on copyModule to remind us later, maybe something like TODO: merge this with copyModuleItems, and add options for copying exports and other things that are currently different between them, if we still need those differences.

kripken · 2024-04-05T17:59:58Z

test/unit/input/gc_target_feature.wasm

Why did this change?

The function name section is no longer written when there is no name.

kripken · 2024-04-08T21:10:22Z

test/lit/wasm-split/jspi.wast

@@ -18,15 +18,15 @@

 ;; PRIMARY:      (import "env" "__load_secondary_module" (func $import$__load_secondary_module (param externref)))

- ;; PRIMARY:      (import "placeholder" "0" (func $placeholder_0 (param i32) (result i32)))
+ ;; PRIMARY:      (import "placeholder" "0" (func $fimport$1 (param i32) (result i32)))


This looks like a loss of a useful name. Perhaps it's better to keep it?

It's useful in tests, but it's not useful for production. Are tests enough motivation?

I think so. In production no names are shipped anyhow so adding more names in non-production builds has little downside, so long as they are meaningful, which looks like the case here.

… an explicit name

sbc100 · 2024-04-12T06:24:27Z

This seems to break some emscripten tests:

======================================================================
FAIL: test_metadce_hello_O0 (test_other.other.test_metadce_hello_O0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/common.py", line 704, in resulting_test
    return func(self, *args)
           ^^^^^^^^^^^^^^^^^
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/test_other.py", line 8522, in test_metadce_hello
    self.run_metadce_test('hello_world.c', *args)
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/test_other.py", line 8457, in run_metadce_test
    self.assertFileContents(filename, data)
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/common.py", line 1387, in assertFileContents
    self.assertTextDataIdentical(expected_content, contents, message,
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/common.py", line 1345, in assertTextDataIdentical
    return self.assertIdentical(text1, text2, msg, fromfile, tofile)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/google/home/sbc/dev/wasm/emscripten/test/common.py", line 1364, in assertIdentical
    self.fail(fail_message)
AssertionError: Unexpected difference:
--- /usr/local/google/home/sbc/dev/wasm/emscripten/test/other/metadce/test_metadce_hello_O0.funcs
+++ /usr/local/google/home/sbc/dev/wasm/emscripten/test/other/metadce/test_metadce_hello_O0.funcs.new
@@ -1,3 +1,5 @@
+$54
+$55
 $__DOUBLE_BITS
 $__ashlti3
 $__emscripten_stdout_close
@@ -26,7 +28,6 @@
 $_emscripten_stack_restore
 $_emscripten_tempret_get
 $_emscripten_tempret_set
-$dynCall_jiji
 $emscripten_stack_get_base
 $emscripten_stack_get_current
 $emscripten_stack_get_end
@@ -41,7 +42,6 @@
 $getint
 $getpid
 $init_pthread_self
-$legalstub$dynCall_jiji
 $main
 $memchr
 $out

For full output run with --verbose.
Run with --rebaseline to automatically update expectations

sbc100 · 2024-04-12T06:24:50Z

Looks like functions that previously had names now just have numbers?

sbc100 · 2024-04-12T06:26:42Z

I guess dymCall-generating pass needs to set hasExplicitName on the functions it creates?

vouillon · 2024-04-12T09:03:42Z

I guess dymCall-generating pass needs to set hasExplicitName on the functions it creates?

Yes, indeed, that would fix the issue. But I'm wondering whether emscripten really needs the internal names. Maybe you can just update the tests? On the other hand, the generated names are meaningful, so it also make sense to fix that in binaryen.

kripken · 2024-04-12T14:54:57Z

Another fix might be to set hasExplicitName by default in the makeFunction constructor, when it receives a name. When a string name is provided, at least, we can use it. Functions not given any name at all in wat or wasm binaries are the main situations where we don't want hasExplicitName set.

@sbc100 I can look into fixing this but I am out today.

sbc100 · 2024-04-12T15:48:38Z

I guess dymCall-generating pass needs to set hasExplicitName on the functions it creates?

Yes, indeed, that would fix the issue. But I'm wondering whether emscripten really needs the internal names. Maybe you can just update the tests? On the other hand, the generated names are meaningful, so it also make sense to fix that in binaryen.

The generated names are indeed meaningful/useful, at least for debugging.

sbc100 · 2024-04-12T15:49:20Z

Another fix might be to set hasExplicitName by default in the makeFunction constructor, when it receives a name. When a string name is provided, at least, we can use it. Functions not given any name at all in wat or wasm binaries are the main situations where we don't want hasExplicitName set.

That sounds like a good fix.

@sbc100 I can look into fixing this but I am out today.

…icitName. NFC This is temporary workaround for the current test failures on the emscripten waterfall. The real fix I believe will be to make `hasExplicitName` the default in these kinds of cases. See: #6466

…icitName. NFC (#6496) This is temporary workaround for the current test failures on the emscripten waterfall. The real fix I believe will be to make `hasExplicitName` the default in these kinds of cases. See: #6466

vouillon · 2024-04-13T14:23:58Z

Another fix might be to set hasExplicitName by default in the makeFunction constructor, when it receives a name. When a string name is provided, at least, we can use it. Functions not given any name at all in wat or wasm binaries are the main situations where we don't want hasExplicitName set.

makeFunction is called with a made-up name in several places, though. For instance, in the binary parser (for imports) and in the legacy text parser.

vouillon commented Apr 3, 2024

View reviewed changes

src/ir/module-splitting.cpp Show resolved Hide resolved

vouillon commented Apr 3, 2024

View reviewed changes

vouillon force-pushed the explicit-names branch 3 times, most recently from 2738190 to 144a4cb Compare April 3, 2024 15:16

vouillon added 2 commits April 3, 2024 17:33

Test component names after a merge

44459f7

wasm-merge: preserve explicit names

e5ba9c2

- only write explicit function names - when merging modules, the name of types, globals and tags in all modules but the first were lost

vouillon force-pushed the explicit-names branch from 144a4cb to e5ba9c2 Compare April 3, 2024 15:34

vouillon commented Apr 3, 2024

View reviewed changes

test/unit/input/gc_target_feature.wasm Outdated Show resolved Hide resolved

kripken reviewed Apr 3, 2024

View reviewed changes

src/ir/module-splitting.cpp Show resolved Hide resolved

kripken reviewed Apr 3, 2024

View reviewed changes

vouillon changed the title ~~wasm-merge: preserve explicit names~~ Fixes regarding explicit names Apr 4, 2024

Set name as explicit when copying a function with a new name

f8c6b7e

kripken reviewed Apr 5, 2024

View reviewed changes

Updates

bae02d1

kripken reviewed Apr 8, 2024

View reviewed changes

Moved back type name copy to module-utils and consider placeholder as…

7afab53

… an explicit name

kripken approved these changes Apr 11, 2024

View reviewed changes

kripken merged commit d662d73 into WebAssembly:main Apr 11, 2024
13 checks passed

vouillon deleted the explicit-names branch April 12, 2024 09:03

sbc100 mentioned this pull request Apr 12, 2024

When creating dynCall and legalstub function mark them as hasExplicitName. NFC #6496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes regarding explicit names #6466

Fixes regarding explicit names #6466

vouillon commented Apr 3, 2024

vouillon Apr 3, 2024

kripken Apr 3, 2024

vouillon Apr 4, 2024

kripken Apr 4, 2024

kripken Apr 5, 2024

vouillon Apr 5, 2024

kripken Apr 5, 2024

vouillon Apr 5, 2024

kripken Apr 8, 2024 •

edited

vouillon Apr 9, 2024

kripken Apr 9, 2024

kripken Apr 5, 2024

vouillon Apr 5, 2024 •

edited

kripken Apr 8, 2024

tlively Apr 8, 2024

kripken Apr 8, 2024

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

vouillon commented Apr 12, 2024

kripken commented Apr 12, 2024

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

vouillon commented Apr 13, 2024

	ret->hasExplicitName = newName.is() \|\| func->hasExplicitName;
	ret->hasExplicitName = func->hasExplicitName;

Fixes regarding explicit names #6466

Fixes regarding explicit names #6466

Conversation

vouillon commented Apr 3, 2024

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 Apr 8, 2024 • edited

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

vouillon Apr 5, 2024 • edited

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

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

vouillon commented Apr 12, 2024

kripken commented Apr 12, 2024

sbc100 commented Apr 12, 2024

sbc100 commented Apr 12, 2024

vouillon commented Apr 13, 2024

kripken Apr 8, 2024 •

edited

vouillon Apr 5, 2024 •

edited