fix: handle duplicate func names in Wasm's name section - #1357
Conversation
b8f5d6a to
f6fd84b
Compare
Miden examples benchmarkCandidate
SVG flamegraphs and compiled packages are attached to the workflow run. |
f6fd84b to
e20b119
Compare
|
The new plan is to address #1360 in this PR here as well, to ensure handling of export names and name-section names works well together. So reverting to draft. |
e20b119 to
25c7d78
Compare
|
This has been reworked to address both #1341 and #1360, as discussed, so it's ready for review. A fix of #1341 is required to make projects using cc @bitwalker, @greenhat |
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looks good, but there are two relatively significant issues that need to be addressed before this can be merged, see comments for details
| }; | ||
| let export_sym = Symbol::intern(export_name.as_str()); | ||
|
|
||
| if exported_as.insert(*func_idx, export_sym).is_some() { |
There was a problem hiding this comment.
Please preserve multiple export aliases for a function instead of rejecting them. This occurs in ordinary optimized Rust: compiling two exported functions foo and bar that both return 42 with rustc -C opt-level=3 --target wasm32-unknown-unknown produces two exports referring to function index 0. This check rejects that output. It also regresses component startup targets with multiple export aliases: I verified that a component whose startup target is exported as both aliased-start and other-start translates on the base commit but fails here. Choose a primary linkage name and preserve the additional exports through aliases or forwarding functions, with a regression test for this case.
| let mut global_names: FxHashSet<Symbol> = FxHashSet::default(); | ||
| for global_idx in self.globals.keys() { | ||
| let name = self.global_name(global_idx); | ||
| if export_names.contains(&name) { |
There was a problem hiding this comment.
Please disambiguate the internal global linkage name instead of rejecting the public function export. For example, this valid module translates on the base commit but fails here:
(module $counter_module
(global $counter (mut i32) (i32.const 0))
(func $read_counter (export "counter") (result i32)
global.get $counter))The global's name-section name and the function's export name are allowed to coincide; the private global's source name should not constrain the public interface. Extend the source/linkage distinction to globals so the public export keeps its name and references to the renamed global remain consistent, and cover this with a regression test.
There was a problem hiding this comment.
Following our sync on Monday, my understanding is that for now we don't expect Wasm producers to generate collisions between global and export names. The code in this PR already emits a diagnostic in that case and as discussed I've opened #1390 if we need this later.
2 participants
Closes #1341
Closes #1360
Both are related to function names and fixes for each must work well in combination.
Summary
low_pcinstead of (non-unique)source_name.Related follow-up work
@source_namefor procedures with divergent source/linkage name #1351miden-vm >= 0.30Note on testing
Duplicate name-section names are easy to reproduce with
miden-field, see #1341. Withoutmiden-fieldit's tricky. Sincemiden-fieldmay change, I avoided it in the test and went for synthetic DWARF instead.