This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cxxfilt] Correctly demangle COFF import thunk
ClosedPublic

Authored by steven_wu on Dec 12 2019, 9:22 AM.

Details

Summary

llvm-cxxfilt wasn't correctly demangle COFF import thunk in those two
cases before:

  • demangle in split mode (multiple words from commandline)
  • the import thunk prefix was added no matter the later part of the

string can be demangled or not
Now llvm-cxxfilt should handle both case correctly.

Event Timeline

steven_wu created this revision.Dec 12 2019, 9:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson added inline comments.Dec 13 2019, 2:21 AM
llvm/test/tools/llvm-cxxfilt/coff-import.test
4–8

I'm not sure what this string is trying to say? If demangling fails, then it doesn't demangle, it just prints the original string.

Also minor nit for consistency with tests that use '#' at the start of RUN and CHECK lines: many of the newer tests for the LLVM binutils use '##' for comments to make them stand out, please use that here.

5

What is this line trying to check? If it is trying to show that "import thunk for" is not printed, then the check is insufficient - you need to show that the check is for the whole line using --match-full-lines. Did this test fail before your change?

7–8

This isn't a "split" demangling as you call it. This is two separate strings ("impZSt6futureIvE" and "impZSt6futureIvE"), which are individually demangled with no string splitting occurring. Did that case fail before?

I think what you need is RUN: llvm-cxxfilt -n "__imp__ZSt6futureIvE __imp__ZSt6futureIvE" so that the two names are treated as a single argument.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
77

Why did you change this name?

jhenderson added inline comments.Dec 13 2019, 2:23 AM
llvm/test/tools/llvm-cxxfilt/coff-import.test
7–8

("impZSt6futureIvE" and "impZSt6futureIvE")

This should say ("__imp__ZSt6futureIvE" and "__imp__ZSt6futureIvE") but Phabricator refrormatted it...

steven_wu marked 4 inline comments as done.Dec 13 2019, 9:33 AM
steven_wu added inline comments.
llvm/test/tools/llvm-cxxfilt/coff-import.test
4–8

I don't want to reformat the other test cases now.

5

It was demangled to "import thunk for _foo" but it shouldn't demangle. --match-full-lines isn't strictly needed for this but adding that can prevent other kind of breakage.

7–8

You are correct that I should test the split mode instead. The old behavior is wrong because the old demangle function prints the prefix directly to OS so the output is interleaving.

echo "__imp__ZSt6futureIvE __imp__ZSt6futureIvE" | ./bin/llvm-cxxfilt -n
import thunk for import thunk for std::future<void> std::future<void>

I will update to test the split mode instead.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
77

I remove the OS from the function signature because it is no longer needed. Now it conflicts with the llvm::demangle in
llvm/Demangle/Demangle.h.

Alternative is to call ::demangle on the callside. I am fine with either.

steven_wu updated this revision to Diff 233822.Dec 13 2019, 9:33 AM

Address review feedback

jhenderson accepted this revision.Dec 16 2019, 1:37 AM

LGTM, with or without the function naming, as you wish, but please make the comment change as requested.

llvm/test/tools/llvm-cxxfilt/coff-import.test
4–8

Sorry if I was unclear: I'm not asking you to reformat the other test cases, just the new comment you are adding. In other words, this line only should go from '#' -> '##'

5

Ah, yes, you're right. I think I must have misunderstood something somewhere.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
77

Ah, I didn't realise that there was a name clash. I'm happy with either approach, whichever you prefer.

This revision is now accepted and ready to land.Dec 16 2019, 1:37 AM
This revision was automatically updated to reflect the committed changes.