Re: [PATCH v1] perf symbol: Add rust v0 demangling support

[Ian Rogers]

On Thu, Jan 30, 2025 at 11:35 AM Daniel Xu <dxu@xxxxxxxxx> wrote:
>
> Hi Ian,
>
> Thanks so much for working on this!
>
> Only did a quick skim. Seems like it'd be nice to reuse the official
> demangler now that we know about it. Will be interesting to
> see how that gets packaged for distros.

I think the best solution is to use the official demangler but in
general we'd like fewer rather than more library dependencies. For
example your initial comments were to work to make the coupling with
capstone and LLVM disassemblers more loose via dlopen:
https://lore.kernel.org/lkml/20250122174308.350350-1-irogers@xxxxxxxxxx/

Thinking through the options (note I'm not doing Rust things and so my
level of skin in the game is minimal):
1) stay as we are - well this is a mess as there is no v0 support and
the legacy support comes from libbfd/libiberty.
2) option (1) plus this change cleaned up - there's still the mess
with the legacy support, which is probably more important given v0
isn't widely adopted.
3) wait for the official demangler to be packaged as a library and
depend upon it, add testing from here - solves legacy and v0 issues,
but brings in an extra library dependency to the build. Timeline not
clear.
4) "contribute"/copy-paste the official demangler into perf tree and
do like (3). Perhaps remove the forked file when a dependable library
exists (there's some precedent for this with how libtraceevent was in
and then removed from the linux tree) - solves the timeline, not great
to fork the official demangler, so far not heard from the author Ariel
Ben-Yehuda and I would strongly prefer they contribute rather than I
copy-paste it.

For (3) and (4) it is probably a good idea to engage the Rust folks in:
https://github.com/rust-lang/rust/issues/60705

Having broken Rust demangling seems like a high priority issue for the
perf tool.

[snip]
> On Wed, Jan 29, 2025, at 11:30 AM, Ian Rogers wrote:
> > +char *rust_v0_bdemangle_sym(const char *str)
>
> Typo? Extra `b`.

Ack. Will fix it but let's figure out if this code is even a plan.

[snip[
> > +     if (demangled) {
> > +             /* Legacy rust demangling input is already C++ demangled. */
> > +             if (rust_is_mangled(demangled))
> > +                     rust_demangle_sym(demangled);
>
> In util/demangle-rust.c, I see:
>
>      * INPUT:
>      *     sym: symbol that has been through BFD-demangling
>
> Perhaps double check that cxxabi is sufficient and update the comment?

I've not tested this change beyond making it pass the tests the change
contains, that are based on the v0 specification's examples. I believe
this/legacy may be working as most C++ implies libstdc++ which I
believe uses libiberty in cxxabi, so BFD demangling. Given the
popularity of libstdc++ I imagine things like LLVM's libcxx will try
to be compatible. Perhaps if there are examples of legacy Rust symbols
we can add a test?

Thanks,
Ian