add demangling of V0 Rust symbols #1070

arielb1 · 2024-12-01T17:49:07Z

Description

Uses the code from https://github.com/rust-lang/rustc-demangle/pull/75/files to allow demangling V0 ("new-style") Rust symbols

Motivation and context

Currently, async-profiler is unable to demangle "new-style" Rust symbols. This PR adds a way of demangling them.

How has this been tested?

Currently, the demangler has been fuzzed against the official Rust demangler and found to be byte-to-byte correct. Still waiting to test this PR in the real world (cc @Mark-Simulacrum).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apangin · 2024-12-02T15:51:13Z

src/demangle.cpp

+    struct demangle demangle;
+    rust_demangle_demangle(s, &demangle);
+    if (rust_demangle_is_known(&demangle)) {
+        return demangleRust(s, &demangle);


Why is demangling performed in two steps? Can be the logic encapsulated in one call?

because that's how it's done in the Rust code and the C code is very based on the Rust code

Do we want to keep this similarity? In any case, I'd expect to have this logic encapsulated in demangleRust rather than in a more generic Demangle::demangle.

demangle.cpp is supposed to be in rust-lang/rustc-demangle#75

Do we want to keep this similarity? In any case, I'd expect to have this logic encapsulated in demangleRust rather than in a more generic Demangle::demangle.

The code being this way means we don't try to demangle invalid Rust symbols as C++. I think that is correct.

apangin · 2024-12-02T15:53:48Z

src/rust-demangle.cpp

+    if (s_len >= 2 && !strncmp(s, "_R", strlen("_R"))) {
+        inner = s+2;
+        inner_len = s_len - 2;
+    } else if (s_len >= 1 && !strncmp(s, "R", strlen("R"))) {
+        inner = s+1;
+        inner_len = s_len - 1;
+    } else if (s_len >= 3 && !strncmp(s, "__R", strlen("__R"))) {


Why are 3 options? Don't V0 symbols always start with _R?

weird MSVC/Mac behavior, not relevant for rust-profiler

Async-profiler already strips the first _ when parsing Mach-O symbols. And it will not support MSVC any time soon.

I want to keep the code synced with rust-lang/rustc-demangle#75

apangin · 2024-12-02T15:54:55Z

src/demangle.cpp

-    return result;
+    free(result);
+    // demangling Rust failed, just return the original string
+    return strdup(s);


The function is supposed to return NULL if it's not a valid Rust symbol. Then the caller attempts to demangle it as a C++ symbol, and if it also fails, returns the original string.

will change

apangin · 2024-12-02T15:58:56Z

src/demangle.cpp

    }
 }

 char* Demangle::demangle(const char* s, bool full_signature) {


Note that Demangle::demangle is called only for symbols starting with _Z.
IIUC, it's not the case for V0 format.

the new revision changes that

krk · 2024-12-02T16:39:38Z

Great work! We could add some tests, using the C++ test runner as in: https://github.com/async-profiler/async-profiler/blob/master/test/native/perfEventsLinuxTest.cpp

arielb1 · 2024-12-02T17:40:34Z

Great work! We could add some tests, using the C++ test runner as in: https://github.com/async-profiler/async-profiler/blob/master/test/native/perfEventsLinuxTest.cpp

Any pointers on this? The original code does not have unit/integration tests.

arielb1 · 2024-12-03T17:24:42Z

added tests

arielb1 · 2024-12-04T11:37:14Z

review ping

krk · 2024-12-04T16:29:51Z

For the testing part of the PR:

Thank you for adding tests! While we don't currently have formal unit testing guidelines, I appreciate your contribution toward improving test coverage.

If needed, you can find coverage reports in the GHA checks as a link, such as in Test and Publish Nightly Builds / test (linux-x64,, corretto 21) (pull_request) under the Upload coverage report step.

We don’t have a specific coverage percentage target, but it would be useful to add tests exercising some of the functions with zero coverage, especially in the following areas:

Punycode-related code: punycode_decode, code_to_utf8, utf8_next_char.
Constants-related: printer_print_const, printer_print_const_str_literal.

arielb1 · 2024-12-04T16:33:56Z

We don’t have a specific coverage percentage target, but it would be useful to add tests exercising some of the functions with zero coverage, especially in the following areas:

There are good tests of that code in https://github.com/rust-lang/rustc-demangle including quite intensive fuzzing, which are written in Rust and therefore would be annoying to copy over

arielb1 · 2024-12-04T16:41:07Z

Copied some of these tests over to increase the coverage

apangin · 2024-12-05T03:10:26Z

src/demangle.cpp

 }

-char* Demangle::demangleRust(const char* s, const char* e) {
+char* Demangle::demangleRust(const char *s, struct demangle const *demangle, bool full_signature) {


Seems like s argument is redundant here: it is only used for initial size estimation. struct demangle already has everything needed to estimate result size even better.

Should I just remove the size estimation logic and start from size 64 and grow binary?

I think that is smarter

apangin · 2024-12-05T03:14:46Z

src/demangle.cpp

-            *r++ = ':';
+    char* result = NULL, *new_result;
+    for (size_t demangled_size = strlen(s) * 3 / 2 + 1; demangled_size < 1000000; demangled_size *= 2) {
+        new_result = (char *)realloc(result, demangled_size);


realloc is not used for its intended purpose (no need to copy previous data).
The code will be even simpler if we use malloc here and free after rust_demangle_display_demangle.

apangin · 2024-12-05T03:31:48Z

src/demangle.cpp

-        }
+    // try to demangle as Rust
+    struct demangle demangle;
+    rust_demangle_demangle(s, &demangle);


This runs some non-trivial pre-parsing. I wonder if this may have performance impact, given that the code is also executed for every C++ symbol.

it should be fairly fast. most C++ symbols don't start with _ZN and a digit, and even if they do it just follows the list of lengths.

most C++ symbols don't start with _ZN and a digit

The majority of JVM symbols start with _ZN and a digit:

nm /usr/java/jdk-23.0.1/lib/server/libjvm.so | grep " _ZN[1-9]" | wc -l 34181

Some of these symbols are demangled as Rust producing a different result, e.g.:

_ZN12_GLOBAL__N_113single_threadE Rust: _GLOBAL__N_1::single_thread CPP: (anonymous namespace)::single_thread

apangin · 2024-12-05T03:37:08Z

src/demangle.h

 #ifndef _DEMANGLE_H
 #define _DEMANGLE_H

+#include "rust-demangle.h"


How exactly Rust symbols are demangled is an implementation detail; I don't think Rust-specific headers should appear in the declaration part of Demangle class. In fact, nothing is really needed from rust-demangle.h here - you can just put a forward declaration of struct demangle.

apangin · 2024-12-05T03:42:39Z

src/demangle.h

  private:
    static char* demangleCpp(const char* s);
-    static char* demangleRust(const char* s, const char* e);
+    static char* demangleRust(const char *s, struct demangle const *demangle, bool full_signature);


I will not ask you to restyle rust-demangle* sources (maybe, except for filenames), since they came from a different project, but please don't change coding style of the existing sources.

apangin · 2024-12-05T03:44:56Z

src/rust-demangle.cpp

+        // Advance by the length
+        for (size_t i = 0; i < len; i++) {
+            if (chars_len == 0) {
+                return DemangleInvalid;
+            }
+            c = *chars++;
+            chars_len--;
+        }


Why doing this char-by-char instead of just incrementing by len at once?

The code is slightly weird due to being copied from Rust code. changed it to be more idiomatic C

changed it to be more C

apangin · 2024-12-05T03:46:31Z

test/native/demangleTest.cpp

+
+// demangle calls _cxa_demangle, which should support demangling Itanium-mangled symbols across all Linux systems,
+// but might have a different mangling format on some other platforms.
+#ifdef __linux__


What is Linux-specific here? Why can't it run on macOS?

I'm not sure if the demangling scheme for MacOS is the same as the one for Linux.

It calls _cxa_demangle and I'm not sure how portable this is

It certainly works on macOS, async-profiler uses it.

apangin · 2024-12-05T03:51:21Z

test/native/demangleTest.cpp

+#ifdef __linux__
+TEST_CASE(Demangle_test_demangle_cpp) {
+    const char *s = Demangle::demangle("_ZNSt15basic_streambufIwSt11char_traitsIwEE9pbackfailEj", false);
+    CHECK_EQ (strcmp(s, "std::basic_streambuf<wchar_t, std::char_traits<wchar_t> >::pbackfail"), 0);


CHECK_EQ supports comparing strings - no need for strcmp.
Also, redundant space before (.

It didn't work for me for some reason and I tried it a few different ways. Do you have an example that works?

An example is

async-profiler/test/native/perfEventsLinuxTest.cpp

Line 16 in f8e887e

CHECK_EQ(event_type->name, name_); \

@krk

I get errors like

actual values: s = 94904844431968 (0x5650C18A3260), "std::basic_streambuf<wchar_t, std::char_traits<wchar_t> >::pbackfail" = 94904675958744 (0x5650B77F7FD8)

It does work if you do it like this:

char *s = Demangle::demangle("_ZNSt15basic_streambufIwSt11char_traitsIwEE9pbackfailEj", false); const char *e = "std::basic_streambuf<wchar_t, std::char_traits<wchar_t> >::pbackfail"; CHECK_EQ(s, e);

Trying to repro, I get this:

const char *a = "asd"; const char *b = "asdf"; CHECK_EQ(a, b); // Assertion failed: (a == b), // actual values: a = "asd", b = "asdf"

Could you share the code for the useless errors you got?

apangin · 2024-12-05T03:51:40Z

test/native/demangleTest.cpp

+TEST_CASE(Demangle_test_demangle_cpp) {
+    const char *s = Demangle::demangle("_ZNSt15basic_streambufIwSt11char_traitsIwEE9pbackfailEj", false);
+    CHECK_EQ (strcmp(s, "std::basic_streambuf<wchar_t, std::char_traits<wchar_t> >::pbackfail"), 0);
+    free((void*)s);


Why not just free(s)?

doesn't compile

found how to fix it

arielb1 · 2024-12-05T13:42:54Z

Just inline one of the string parameters in the tests.On 5 Dec 2024, at 15:40, Kerem Kat ***@***.***> wrote: @krk commented on this pull request. In test/native/demangleTest.cpp:

+ CHECK_EQ(Demangle::needsDemangling("_ZN12panic_unwind3imp5panic17exception_cleanup17he4cf772173d90f46E"), true);

+ // Rust v0-mangled symbol + CHECK_EQ(Demangle::needsDemangling("_RNvCs6KtT2fMGqXk_8infiloop4main"), true); + // C++ symbol + CHECK_EQ(Demangle::needsDemangling("_ZNKSbIwSt11char_traitsIwESaIwEE4_Rep12_M_is_sharedEv"), true); + // C symbols + CHECK_EQ(Demangle::needsDemangling("malloc"), false); + CHECK_EQ(Demangle::needsDemangling("_malloc"), false); +} + +// demangle calls _cxa_demangle, which should support demangling Itanium-mangled symbols across all Linux systems, +// but might have a different mangling format on some other platforms. +#ifdef __linux__ +TEST_CASE(Demangle_test_demangle_cpp) { + const char *s = Demangle::demangle("_ZNSt15basic_streambufIwSt11char_traitsIwEE9pbackfailEj", false); + CHECK_EQ (strcmp(s, "std::basic_streambuf<wchar_t, std::char_traits<wchar_t> >::pbackfail"), 0); Trying to repro, I get this: const char *a = "asd"; const char *b = "asdf"; CHECK_EQ(a, b); // Assertion failed: (a == b), // actual values: a = "asd", b = "asdf" Could you share the code for the useless errors you got? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

krk · 2024-12-05T13:52:05Z

Thanks, fixed in #1072

1. include the hash when doing full-signature demangling 2. fix the C++ vs. Rust heuristic 3. add unit tests

arielb1 · 2024-12-05T16:06:41Z

Updated PR to include the fix + appropriately cleaned unit tests

arielb1 · 2024-12-06T17:16:18Z

So should I keep the old hash heuristic since it seems to work well enough?On 6 Dec 2024, at 19:12, Andrei Pangin ***@***.***> wrote: @apangin commented on this pull request. In src/demangle.cpp:

@@ -117,14 +60,19 @@ void Demangle::cutArguments(char* s) {

} char* Demangle::demangle(const char* s, bool full_signature) { - // Check if the mangled symbol ends with a Rust hash "17h<hex>E" - const char* e = strrchr(s, 'E'); - if (e != NULL && e - s > 22 && e[-19] == '1' && e[-18] == '7' && e[-17] == 'h') { - const char* h = e - 16; - while ((*h >= '0' && *h <= '9') || (*h >= 'a' && *h <= 'f')) h++; - if (h == e) { - return demangleRust(s + 3, e - 19); - } + // try to demangle as Rust + struct demangle demangle; + rust_demangle_demangle(s, &demangle); Some of these symbols are demangled as Rust producing a different result, e.g.: _ZN12_GLOBAL__N_113single_threadE Rust: _GLOBAL__N_1::single_thread CPP: (anonymous namespace)::single_thread —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

arielb1 · 2024-12-06T17:22:22Z

So if a symbol starts with _R or if it ends with a h17…E use the Rust demangling, otherwise use cxa_demangle.This will use cxa_demangle for legacy-mangled Rust symbols that have an LLVM termination, but I think it’s not too bad.On 6 Dec 2024, at 19:16, ***@***.*** wrote:So should I keep the old hash heuristic since it seems to work well enough?On 6 Dec 2024, at 19:12, Andrei Pangin ***@***.***> wrote: @apangin commented on this pull request. In src/demangle.cpp:

@@ -117,14 +60,19 @@ void Demangle::cutArguments(char* s) {

} char* Demangle::demangle(const char* s, bool full_signature) { - // Check if the mangled symbol ends with a Rust hash "17h<hex>E" - const char* e = strrchr(s, 'E'); - if (e != NULL && e - s > 22 && e[-19] == '1' && e[-18] == '7' && e[-17] == 'h') { - const char* h = e - 16; - while ((*h >= '0' && *h <= '9') || (*h >= 'a' && *h <= 'f')) h++; - if (h == e) { - return demangleRust(s + 3, e - 19); - } + // try to demangle as Rust + struct demangle demangle; + rust_demangle_demangle(s, &demangle); Some of these symbols are demangled as Rust producing a different result, e.g.: _ZN12_GLOBAL__N_113single_threadE Rust: _GLOBAL__N_1::single_thread CPP: (anonymous namespace)::single_thread —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

apangin · 2024-12-06T17:25:44Z

if a symbol starts with _R or if it ends with a h17…E use the Rust demangling, otherwise use cxa_demangle

Sounds good to me

arielb1 · 2024-12-06T17:33:50Z

Anything else?

apangin · 2024-12-06T17:48:37Z

Just a minor thing: perhaps, rename rust-demangle.* to rustDemangle.* or something (to match file naming convention of the project), and add a link to https://github.com/rust-lang/rustc-demangle in the source code to make it clear where the code originates from?

arielb1 · 2024-12-06T17:51:40Z

The C code now has a link - 9ac516a#diff-6e3cd5c659e8a31192524378ab2bf3b2a71ffd6693fa346418d045fb29cb3188R9 Will rename the file.

arielb1 · 2024-12-06T19:54:01Z

updated to use the old (same as before this PR) Rust vs. C++ heuristic

arielb1 force-pushed the demangle-rust branch from dfd4712 to d9606a2 Compare December 2, 2024 11:13

apangin reviewed Dec 2, 2024

View reviewed changes

arielb1 force-pushed the demangle-rust branch 4 times, most recently from 5d39cf0 to 24b162a Compare December 2, 2024 16:17

arielb1 force-pushed the demangle-rust branch from 61e044c to f5c6618 Compare December 3, 2024 15:27

arielb1 force-pushed the demangle-rust branch from 604df35 to 3a0d6cd Compare December 4, 2024 16:57

apangin reviewed Dec 5, 2024

View reviewed changes

arielb1 force-pushed the demangle-rust branch from 5bb6fa3 to abecae2 Compare December 5, 2024 11:16

Ariel Ben-Yehuda added 11 commits December 5, 2024 16:04

add demangling of V0 Rust symbols

122996c

actually call the demangling on Rust V0 symbols

a5c93a0

add comment about distinction between FRAME_CPP and FRAME_NATIVE

bec39cc

improve demangling

a7dd259

1. include the hash when doing full-signature demangling 2. fix the C++ vs. Rust heuristic 3. add unit tests

add more tests and cleanup

28c9e97

add more tests for native demangling

38bb8f6

remove printfs

20af1f5

add more tests for native demangling

e283545

better handling of suffixes in Rust demangling

635efcc

update rust-demangle

9ac516a

address review comments

952f525

remove strcmp in unit tests

5cfe5e7

arielb1 force-pushed the demangle-rust branch from abecae2 to 5cfe5e7 Compare December 5, 2024 16:06

rename rust-demangle to rustDemangle to fit naming convention

53096f5

return to the old Rust/C++ heuristic

be35d14

arielb1 force-pushed the demangle-rust branch from fc4319c to be35d14 Compare December 6, 2024 19:54

fix tests on libc++/OS X

34f736f

arielb1 force-pushed the demangle-rust branch from 9c70352 to 34f736f Compare December 6, 2024 20:15

apangin merged commit bce178e into async-profiler:master Dec 6, 2024
9 checks passed

add demangling of V0 Rust symbols #1070

add demangling of V0 Rust symbols #1070

Uh oh!

Conversation

arielb1 commented Dec 1, 2024

Description

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krk commented Dec 2, 2024

Uh oh!

arielb1 commented Dec 2, 2024

Uh oh!

arielb1 commented Dec 3, 2024

Uh oh!

arielb1 commented Dec 4, 2024

Uh oh!

krk commented Dec 4, 2024

Uh oh!

arielb1 commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

arielb1 Dec 2, 2024 •

edited

Loading

arielb1 Dec 2, 2024 •

edited

Loading

arielb1 commented Dec 4, 2024 •

edited

Loading

arielb1 Dec 5, 2024 •

edited

Loading

arielb1 Dec 5, 2024 •

edited

Loading