Use WASM function names in compiled objects

[lpereira]

Instead of generating symbol names in the format
"wasm[$MODULE_ID]::function[$FUNCTION_INDEX]", generate (if possible) something more readable, such as "wasm[$MODULE_ID]::$FUNCTION_NAME". This helps when debugging or profiling the generated code.

[bjorn3]

Is it guaranteed that the names section only contains unique names?

[alexcrichton]

Thanks! This makes sense to me and I've often thought this would be nice as well. A hesitation I've had in the past though is that I'm a bit wary about exposing user-provided data to native tools like perf and objdump and such. The name section has completely arbitrary strings and I while I can't say for certainty that native tooling behaves well in the face of arbitrary strings I suspect that there might be a security issue waiting to happen along the lines of "if you run ldd over a binary it can run arbitrary code" or something like that.

(a good example being @bjorn3's suggestion where the name section has no guarantees of uniqueness and I'm not sure what tooling would do with same-named-symbols)

[cfallin]

I wonder if we could define suitable "cleanup" for names: (i) rewrite duplicates with numeric suffixes, (ii) replace or remove any characters not in the usual set ([A-Za-z0-9-_] -- maybe spaces too? can tools handle those?), (iii) cap the length at something reasonable like 32 or 64 characters?

[lpereira]

Could also add the function_index for the cases where uniqueness isn't guaranteed. Alright, I'll cook up a patch with the suggestions.

[alexcrichton]

Yeah I think that'd work well (albeit bit a bit more involved in the implementation). While I can't say for certainty that spaces are not supported my guess is that tools would likely eventually choke on non-C-looking symbols so I'd be tempted to leave them out.

[fitzgen]

How about

• wasm[N]::func[M] if no name section entry
• wasm[N]::func[M]: <name> where <name> is sanitized and has a capped length if there is an entry

?

This avoids needing to either add numbers into the sanitized name and then worrying about removing those numbers when capping the length, or needing to account for the length of the numbers that might be added after sanitizing and capping the name.

[lpereira]

Alright, pushed some changes that:

• Will filter symbols to only include alphanumeric characters and others that might be generated by name mangling and whatnot, and replace runs of them with a single '?'
• Always include the function index in the symbol name
• Truncate the symbol name to 96 characters

I think this should be sufficient. Can anyone please take a look again?

[cfallin]

.take(N) in the iterator chain should allow the truncation to happen inline (and stop iteration early if so, since the whole chain is lazy)

[lpereira]

Done.

[lpereira]

Actually, no, I'm not sure how to do this; I want to truncate the string after trim_matches('?') removed repeated occurrences of '?' in the cleaned string, not before.

[jameysharp]

You mention that you intended to turn any run of multiple ? into a single ?. Note that trim_matches doesn't do that. Instead, it deletes all copies of the given pattern from both ends of the string. That's why it can return a borrowed &str instead of a heap-allocated String: it doesn't change the middle of the string, so it can just adjust the start pointer and the length.

I think this should do more or less what you want (though I'm typing it in GitHub's web editor without testing it so who knows). It deletes all ? at the beginning of the string, then compresses any run of multiple ? into a single ?. It can leave a single ? at the end of the string but I couldn't think of a simple way to fix that and I don't think it matters. Also it uses take like Chris suggested.

            let mut last_char = '?';
            let name = name
                .chars()
                .map(|c| if bad_char(c) { '?' } else { c })
                .filter(|c| { let keep = last_char != c; last_char = c; keep })
                .take(Self::MAX_SYMBOL_LEN)
                .collect::<String>();

[cfallin]

Looks reasonable to me, thanks for the iterations!

[cfallin]

Ah, force-pushed after my review; could you do subsequent updates as additional commits? Let me know when stable and I can re-review.

[lpereira]

It's stable now. (I'm not using the take() method for the reason I stated above, so I reverted to the previous version.)

[jameysharp]

This is going to be such a big usability improvement for people using tools like perf! I'm excited.

[jameysharp]

You mention that you intended to turn any run of multiple ? into a single ?. Note that trim_matches doesn't do that. Instead, it deletes all copies of the given pattern from both ends of the string. That's why it can return a borrowed &str instead of a heap-allocated String: it doesn't change the middle of the string, so it can just adjust the start pointer and the length.

I think this should do more or less what you want (though I'm typing it in GitHub's web editor without testing it so who knows). It deletes all ? at the beginning of the string, then compresses any run of multiple ? into a single ?. It can leave a single ? at the end of the string but I couldn't think of a simple way to fix that and I don't think it matters. Also it uses take like Chris suggested.

            let mut last_char = '?';
            let name = name
                .chars()
                .map(|c| if bad_char(c) { '?' } else { c })
                .filter(|c| { let keep = last_char != c; last_char = c; keep })
                .take(Self::MAX_SYMBOL_LEN)
                .collect::<String>();

[jameysharp]

Ah, the CI test failures are because tests/disas.rs is specifically looking for symbol names which contain both wasm and function in their names, and you've shortened the latter to func. This is near the beginning of disas_elf.

I don't like that this test is relying on this implementation detail of Wasmtime, but I suppose it's fine. So you can either change this PR to continue to use function, or change disas.rs to search for func.

Either way, you'll also need to run WASMTIME_TEST_BLESS=1 cargo test --test disas to update the tests to reflect this change. Some of them do have function names, and those now show up in the expected test output, which is a fantastic bonus.

[lpereira]

Alright, thanks for the review! This should be the last round.

(@jameysharp, that filter wouldn't work -- words like "keep", which are fine, would be transformed to "kep" -- but I tweaked it to consider that case.)

[jameysharp]

(@jameysharp, that filter wouldn't work -- words like "keep", which are fine, would be transformed to "kep" -- but I tweaked it to consider that case.)

Oh, you're absolutely right. 😅

Looks good to me, let's ship it!