Page MenuHomePhabricator

[llvm-cxxfilt] Split and demangle stdin input
ClosedPublic

Authored by mattd on Jan 28 2019, 11:50 AM.

Details

Summary

Originally, llvm-cxxfilt would treat a line as a single mangled item to be demangled.
If a mangled name appears in the middle of that string, that name would not be demangled.

GNU c++filt splits and demangles every word in a string that is piped to it via stdin.
Prior to this patch llvm-cxxfilt would never split strings piped to it.
This patch replicates the GNU behavior and splits strings that are piped to it via stdin.

This fixes PR39990

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.Jan 28 2019, 11:50 AM
jhenderson added inline comments.
llvm/test/tools/llvm-cxxfilt/types.test
2 ↗(On Diff #183917)

You probably don't need this test, as it doesn't test any different functionality than the first test.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
9–11 ↗(On Diff #183917)

I'm not sure you need (some of) these headers, even if you use them, by my understanding of the coding guidelines, because e.g. CommandLine.h includes them: https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible

85 ↗(On Diff #183917)

Mangled should be a StringRef.

compnerd requested changes to this revision.Jan 29 2019, 8:41 AM

This actually breaks the compatibility with the existing tools. The quoted string is supposed to be interpreted as a *single* entry. I think that if you want to support something like this, it should be a separate flag for this behaviour.

This revision now requires changes to proceed.Jan 29 2019, 8:41 AM

This actually breaks the compatibility with the existing tools. The quoted string is supposed to be interpreted as a *single* entry. I think that if you want to support something like this, it should be a separate flag for this behaviour.

Which tools are you referring to? This matches GNU c++filt behaviour for some cases at least:

C:\Work>c++filt "abc _Z3foov def"
abc _Z3foov def

C:\Work>c++filt
abc _Z3foov def // stdin
abc foo() def

C:\Work>nm bar.elf | c++filt
0000000000000000 t foo()

It looks like the behaviour is different depending on whether this is fed in via stdin or via the command-line.

mattd marked an inline comment as done.Jan 29 2019, 9:07 AM

This actually breaks the compatibility with the existing tools. The quoted string is supposed to be interpreted as a *single* entry. I think that if you want to support something like this, it should be a separate flag for this behaviour.

That's a fair point. I'll respin the patch with a flag. Thanks for taking a look.

llvm/test/tools/llvm-cxxfilt/types.test
2 ↗(On Diff #183917)

You are correct, this test is similar to the earlier test, but I wanted to the added functionality against the '-t' option as well.

mattd marked 2 inline comments as done.Jan 29 2019, 9:33 AM
mattd added inline comments.
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
9–11 ↗(On Diff #183917)

I can indirectly include SmallVector and StringRef. I'll still need to include StringExtras.

mattd updated this revision to Diff 185865.Feb 7 2019, 2:02 PM
mattd marked an inline comment as done and an inline comment as not done.
mattd retitled this revision from [llvm-cxxfilt] Split input and demangle each word. to [llvm-cxxfilt] Split and demangle stdin input.
mattd edited the summary of this revision. (Show Details)

I've updated the patch to replicate GNU's c++filt behavior. This patch now only splits strings that are passed via stdin. It does not split strings that are passed as arguments to llvm-cxxfilt via command line.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 2:02 PM
jhenderson accepted this revision.Feb 8 2019, 1:56 AM

This essentially looks good to me, aside from the extra test suggestion. I'm away for a week after today, so I won't be able to respond to further review issues until I'm back. I'm happy for this to land once @compnerd is.

llvm/test/tools/llvm-cxxfilt/simple.test
2–3 ↗(On Diff #185865)

It may be worth adding a test to show that reading this from the command-line doesn't cause it to be demangled.

compnerd accepted this revision.Feb 11 2019, 10:48 AM

LG with the additional test suggestion

This revision is now accepted and ready to land.Feb 11 2019, 10:48 AM
This revision was automatically updated to reflect the committed changes.