Support multiple clone suffixes #194

jan-auer · 2020-04-07T18:01:59Z

Fixes demangling errors with symbols that contain multiple clone suffixes, as well as clone suffixes without numbers. Such symbols are quite frequent in ELF symbol tables and they currently fail to demangle completely.

Examples are taken from a Google Breakpad Linux build, as well as from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40831.

Fixes #193

jan-auer · 2020-04-07T18:07:01Z

Note that c++filt and llvm-cxxfilt resolve the clone suffixes differently than cpp_demangle does - I have not tried with GCC:

llvm-cxxfilt:  (.isra.20.part.21)
cpp_demangle:  [clone .isra.20 .part.21]

I chose to leave a space between the clone suffixes for readability.

Saldivarcher · 2020-04-14T22:32:01Z

Awesome work! Personally I think we should probably stay closer to matching libiberty rather than llvm-cxxfilt. Would you mind trying to get this PR to match libiberty?

$  c++filt --version                                                                                                                                                                                                            
GNU c++filt (GNU Binutils) 2.34
Copyright (C) 2020 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) any later version.
This program has absolutely no warranty.

$ c++filt _Z3fooi.part.9.165493.constprop.775.31805                                                                                                                                                                             
foo(int) [clone .part.9.165493] [clone .constprop.775.31805]

So looking at the output it looks like it separates each suffix into their own bracket. Also if you get that fixed, could you make _Z3fooi.part.9.165493.constprop.775.31805 part of the libiberty tests?

jan-auer · 2020-04-15T09:11:37Z

@miguelsaldivar updated, thanks!

jan-auer · 2020-04-15T09:26:13Z

I'm noticing now that there is another test that's failing from the libiberty test suite:

_Z3fooi.1988
foo(int) [clone .1988]

I'm wondering now what the best approach to parse this. We could detect that the clone type identifier is missing and just make it empty or optional. WDYT?

Saldivarcher · 2020-04-15T21:45:46Z

src/ast.rs

@@ -7791,7 +7795,7 @@ mod tests {
                                        SourceName(Identifier {
                                            start: 3,
                                            end: 6,
-                                        }))))), None),
+                                        }))))), Vec::new()),


I think we need vec![] rather than Vec::new(), to satisfy the no-std requirement? That's where the CI is failing.

Good point. I was wondering about CI already, it seems that external contributors cannot see CI...

I can't see it through there either for some reason, but going here does the trick.

travis notifications have been flaky in gimli as well.

I wonder if this started happening after the project was moved to gimli-rs? Could be an old webhook, potentially.

Saldivarcher · 2020-04-15T21:52:51Z

$ c++filt _Z3fooi.1988
foo(int) [clone .1988]
$ cppfilt _Z3fooi.1988
foo(int) [clone .1988]


$ c++filt -p _Z3fooi.1988
foo
$ cppfilt -p _Z3fooi.1988
foo [clone .1988]

On my local machine, and on CI all the tests seem to be passing. The only differences I see are with the --no-params flag.

Saldivarcher · 2020-04-15T22:01:05Z

@jan-auer I think once you get a fix for that no-params issue, you should be good to go :)

jan-auer · 2020-04-15T22:17:01Z

src/ast.rs

-        let (identifier, tail) = CloneTypeIdentifier::parse(ctx, subs, tail)?;
-        if tail.is_empty() {
-            return Err(error::Error::UnexpectedText);
-        }


On my local machine, and on CI all the tests seem to be passing

That's because I had this one test disabled. I didn't look into it too closely, but the cppfilt example does demangle, while the libiberty tests did not. They all ran into this UnexpectedText error, since the symbol foo(int) [clone .1988] only has a nonnegative number but no type identifier.

Coming back to my earlier message: According to the grammar, the clone type identifier is optional:

<clone-suffix> ::= [ . <clone-type-identifier> ] [ . <nonnegative number> ]*

With this fix, the number is parsed into the clone type identifier. For demangling, this doesn't really make a difference. However, if you prefer to make the clone type identifier optional, I can do that aswell.

Okay I see now. Making it optional seems like the way to go. c++filt's output seems to not care if you're missing either of those, but if you're missing both that's when c++filt does nothing.

$ c++filt _Z3fooi.12 foo(int) [clone .12] $ c++filt _Z3fooi.part foo(int) [clone .part] $ c++filt _Z3fooi. _Z3fooi.

I think making it optional is the best choice imo. :)

I'm trying this now, and I'm no longer sure if it's actually worth the effort. This complicates parsing quite a bit for very little benefit. I've pushed this up in a separate commit now to see the difference. I'd highly be in favor of leaving the earlier version, though.

The problem with this implementation is that we have to detect a number of cases throughout the hierarchy of CloneSuffix and CloneTypeIdentifier. This creates very tight coupling, since we have to swallow errors in zero_or_one, and then reintroduce redundant checks at the higher level. For example, see fbd5741#diff-6a87164025de4254d2818e3ca4bc813aR1538.

libiberty Does not seem to differentiate, and instead just consumes everything into one large string. In that sense, the implementation we had earlier is probably enough and serves the purpose.

Yeah you're right it does add a bit more overhead, and doesn't seem worth it. Okay I think just making it empty should be fine.

Thanks. Reverted to the prior implementation.

Sorry for making you jump through those hoops, awesome work though!

No worries, it's well worth the effort to get a good implementation :)

Saldivarcher · 2020-04-18T01:17:50Z

@fitzgen can you merge, I don't have access. Thank you! :)

philipc · 2020-04-21T23:57:20Z

I don't think CI ran for the last push (9059eac) but I can't see a way to retry it. Going to merge as is...

Saldivarcher · 2020-04-22T01:33:01Z

@philipc thank you for merging! I appreciate it :)

jan-auer · 2020-04-22T15:34:44Z

Thank you both! I'd appreciate a release with this commit so I can publish a dependent crate.

jan-auer force-pushed the fix/multiple-suffixes branch from 57776db to 9bf586a Compare April 7, 2020 18:20

This was referenced Apr 7, 2020

build(demangle): Bump cpp_demangle to fix clone suffixes getsentry/symbolic#215

Merged

build: Temporarily use symbolic branch getsentry/symbolicator#211

Merged

fitzgen requested a review from Saldivarcher April 14, 2020 15:26

jan-auer force-pushed the fix/multiple-suffixes branch from 9bf586a to d6225c6 Compare April 15, 2020 09:10

jan-auer force-pushed the fix/multiple-suffixes branch from d6225c6 to 46aab00 Compare April 15, 2020 09:21

Saldivarcher reviewed Apr 15, 2020

View reviewed changes

jan-auer force-pushed the fix/multiple-suffixes branch from 46aab00 to dbddf4d Compare April 15, 2020 21:54

Support multiple clone suffixes

9059eac

jan-auer force-pushed the fix/multiple-suffixes branch from dbddf4d to 9059eac Compare April 15, 2020 22:12

jan-auer commented Apr 15, 2020

View reviewed changes

jan-auer force-pushed the fix/multiple-suffixes branch from fbd5741 to 9059eac Compare April 17, 2020 07:24

philipc merged commit d007949 into gimli-rs:master Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple clone suffixes #194

Support multiple clone suffixes #194

jan-auer commented Apr 7, 2020 •

edited

Loading

jan-auer commented Apr 7, 2020

Saldivarcher commented Apr 14, 2020

jan-auer commented Apr 15, 2020

jan-auer commented Apr 15, 2020

Saldivarcher Apr 15, 2020

jan-auer Apr 15, 2020

Saldivarcher Apr 15, 2020

philipc Apr 15, 2020

jan-auer Apr 17, 2020

Saldivarcher commented Apr 15, 2020

Saldivarcher commented Apr 15, 2020

jan-auer Apr 15, 2020 •

edited

Loading

Saldivarcher Apr 16, 2020

jan-auer Apr 16, 2020 •

edited

Loading

Saldivarcher Apr 16, 2020

jan-auer Apr 17, 2020

Saldivarcher Apr 18, 2020

jan-auer Apr 20, 2020

Saldivarcher commented Apr 18, 2020

philipc commented Apr 21, 2020

Saldivarcher commented Apr 22, 2020

jan-auer commented Apr 22, 2020

Support multiple clone suffixes #194

Support multiple clone suffixes #194

Conversation

jan-auer commented Apr 7, 2020 • edited Loading

jan-auer commented Apr 7, 2020

Saldivarcher commented Apr 14, 2020

jan-auer commented Apr 15, 2020

jan-auer commented Apr 15, 2020

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

Saldivarcher commented Apr 15, 2020

Saldivarcher commented Apr 15, 2020

jan-auer Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer Apr 16, 2020 • edited Loading

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

Saldivarcher commented Apr 18, 2020

philipc commented Apr 21, 2020

Saldivarcher commented Apr 22, 2020

jan-auer commented Apr 22, 2020

jan-auer commented Apr 7, 2020 •

edited

Loading

jan-auer Apr 15, 2020 •

edited

Loading

jan-auer Apr 16, 2020 •

edited

Loading