demangle: More elaborated demangling for Rust #1625

ChoKyuWon · 2023-01-29T21:44:08Z

This commit fixes following things:

Ignore traits (See Rust symbol demangling for Traits #1620)
Demangle '..' to '::'
Consume extra '_' at starting point of string
Add placeholder to support Rust v0 mangling rule (start with '_R')

However, because of 2, it replace the '..' to '::', it need a write permission for the source string. It makes segfault for the demangle test because the source string is const.

Signed-off-by: ChoKyuWon kyuwoncho18@gmail.com

namhyung

Thanks for the PR. Can you please split the ".." part and trait handling?

utils/demangle.c

namhyung

Can you rewrite the header line of the second commit message? The same message confuses readers and I'd prefer being more specific like 'demangle: convert ".." to "::" for Rust'.

namhyung · 2023-02-01T04:36:36Z

utils/demangle.c

 		num = dollar - p;
-		dd_append_len(dd, p, num);
+		/* consume extra '_' before '$' */


I'm not sure about this. How can we make sure if it's not a valid identifier. For example it might have my_mod_ as a module name, then any dollar sign after that would delete the trailing underscore, right?

That's a good point. I'll revert this code. However, for clearer context, the first underscore would be deleted IMO.

namhyung · 2023-02-01T04:38:31Z

utils/demangle.c

+			return xstrdup(str);
+	}
+
+	// TODO: implement demangling Rust v0 mangling


Thanks for doing this. Could you please create an issue with this? It'd be great if it can have some examples.

namhyung · 2023-02-01T04:40:06Z

utils/demangle.c

@@ -1469,6 +1470,11 @@ static int dd_source_name(struct demangle_data *dd)
 	if (add_name)
 		dd_append_len(dd, p, num);
 	__dd_consume_n(dd, num, NULL);
+	separator = strstr(dd->new, "..");


I'm afraid it might have multiple "..". Does this handle it properly?

I'm not sure why, but in my test case (position at last of demangle_rust1), it seems to work properly. But to make sure, I'll add a loop for that.

I think it worked by luck. It just converts a single instance, but later dd_source_name() call would convert it again. If the original name contains more than two ".." and it has a dd_source_name() after that, not all ".." would be converted.

You're right. Thanks for the good point.

namhyung

Please add some explanation in the second commit message. Like when it needs to be converted or just include an example.

Also I think we should add a test for more complex case like in the #1620.

namhyung · 2023-02-01T05:32:01Z

utils/demangle.c

+			separator[1] = ':';
+		}
+		else
+			break;


In general, I prefer reducing the nesting level by checking the break condition early. Something liike below..

while (true) { separator = strstr(dd->new, ".."); if (separator == NULL) break; separator[0] = ':'; separator[1] = ':'; }

namhyung · 2023-02-01T05:32:52Z

utils/demangle.c

@@ -1382,6 +1382,7 @@ static int dd_source_name(struct demangle_data *dd)
 {
 	int num = dd_number(dd);
 	char *dollar;
+	char *separator;


You may put it inside the loop below.

namhyung · 2023-02-01T05:36:34Z

utils/demangle.c

+		}
+		else
+			break;
+	}


Another concern is that it's suboptimal. It checks ".." whenever it calls dd_source_name(). I think we can do it only when the dollar sign was found. Also it doesn't need to search the whole (new) string. Could we check the newly added region only?

I'm not sure that the ".." is always inside the <>. I'll try to find a counterexample, and if there's nothing, I'll fix this commit.

I collect the LLVM IR of large-scale projects and search the pattern like this:

$ cat *.ll | grep "define[^w]*\.\." | grep -v "$"

and it returns nothing.
So ".." only generated with a dollar sign is quite a safe guess. I'll work on the optimal solution.

namhyung

Thanks for your work. I only have a few nitpicks.

namhyung · 2023-02-05T02:25:35Z

utils/demangle.c

@@ -1426,7 +1426,6 @@ static int dd_source_name(struct demangle_data *dd)
 	/* check special symbol mappings (e.g. '$LT$') for Rust */
 	while (dollar != NULL && dollar < end) {
 		bool found = false;
-


Unnecessary change. Please leave it.

namhyung · 2023-02-05T02:28:42Z

utils/demangle.c

 	DEMANGLE_TEST("_ZN3foo3bar17h05af221e174051e9E", "foo::bar");
+	DEMANGLE_TEST(
+		"_ZN61_$LT$$RF$std..io..stdio..Stdout$u20$as$u20$std..io..Write$GT$9write_fmt17h75c561f414a62159E",
+		"_<&std::io::stdio::Stdout>::write_fmt");


This should be moved to the next commit.

namhyung · 2023-02-05T02:29:33Z

utils/demangle.c

@@ -1426,8 +1426,17 @@ static int dd_source_name(struct demangle_data *dd)
 	/* check special symbol mappings (e.g. '$LT$') for Rust */
 	while (dollar != NULL && dollar < end) {
 		bool found = false;
+		char *separator = p;


The general guide is to have a blank line between declaration and body.

namhyung

LGTM. Can you please rebase it onto the current master?

namhyung · 2023-02-06T02:37:10Z

You need to add a blank line after the commit header to separate subject and body. Please take a look at other commit messages.

This commit fixes following things: 1. Ignore traits (See #1620) 2. Add placeholder to support Rust v0 mangling rule (start with '_R') (See #1626) Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>

For Rust mangling, they use ".." as separator. This commit replace it to standard separator for more intuitive transformation. Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>

namhyung reviewed Jan 30, 2023

View reviewed changes

utils/demangle.c Outdated Show resolved Hide resolved

namhyung reviewed Feb 1, 2023

View reviewed changes

namhyung reviewed Feb 5, 2023

View reviewed changes

namhyung approved these changes Feb 5, 2023

View reviewed changes

ChoKyuWon added 2 commits February 6, 2023 13:24

demangle: More elaborated demangling for Rust

2f53972

This commit fixes following things: 1. Ignore traits (See #1620) 2. Add placeholder to support Rust v0 mangling rule (start with '_R') (See #1626) Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>

demangle: convert ".." to "::" for Rust

195945d

For Rust mangling, they use ".." as separator. This commit replace it to standard separator for more intuitive transformation. Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>

namhyung merged commit 2083cdc into namhyung:master Feb 6, 2023

honggyukim added the language support label Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

demangle: More elaborated demangling for Rust #1625

demangle: More elaborated demangling for Rust #1625

ChoKyuWon commented Jan 29, 2023

namhyung left a comment

namhyung left a comment

namhyung Feb 1, 2023

ChoKyuWon Feb 1, 2023

namhyung Feb 1, 2023

namhyung Feb 1, 2023

ChoKyuWon Feb 1, 2023 •

edited

namhyung Feb 1, 2023

ChoKyuWon Feb 1, 2023 •

edited

namhyung left a comment

namhyung Feb 1, 2023

namhyung Feb 1, 2023

namhyung Feb 1, 2023

ChoKyuWon Feb 1, 2023

ChoKyuWon Feb 1, 2023

namhyung left a comment

namhyung Feb 5, 2023

namhyung Feb 5, 2023

namhyung Feb 5, 2023

namhyung left a comment

namhyung commented Feb 6, 2023

demangle: More elaborated demangling for Rust #1625

demangle: More elaborated demangling for Rust #1625

Conversation

ChoKyuWon commented Jan 29, 2023

namhyung left a comment

Choose a reason for hiding this comment

namhyung left a 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

ChoKyuWon Feb 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChoKyuWon Feb 1, 2023 • edited

Choose a reason for hiding this comment

namhyung left a 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

namhyung left a 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

namhyung left a comment

Choose a reason for hiding this comment

namhyung commented Feb 6, 2023

ChoKyuWon Feb 1, 2023 •

edited

ChoKyuWon Feb 1, 2023 •

edited