[llvm-cxxfilt] Correctly demangle COFF import thunk

[steven_wu]

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.

[jhenderson]

llvm/test/tools/llvm-cxxfilt/coff-import.test

4

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]

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]

llvm/test/tools/llvm-cxxfilt/coff-import.test

4

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]

Address review feedback

[jhenderson]

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

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.

llvm/test/tools/llvm-cxxfilt/coff-import.test

 RUN: llvm-cxxfilt -_ ___imp__ZSt6futureIvE | FileCheck %s
 RUN: llvm-cxxfilt -n __imp__ZSt6futureIvE | FileCheck %s
 
-CHECK: import thunk for std::future<void>
+# This should demangle to origin string
+RUN: llvm-cxxfilt -n __imp__foo | FileCheck %s --check-prefix=CHECK-STRING
+
+RUN: llvm-cxxfilt -n __imp__ZSt6futureIvE __imp__ZSt6futureIvE | \
+RUN:    FileCheck %s --check-prefix=CHECK-SPLIT
 
+CHECK: import thunk for std::future<void>
+CHECK-STRING: __imp__foo
+CHECK-SPLIT: import thunk for std::future<void>
+CHECK-SPLIT-NEXT: import thunk for std::future<void>

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

if (StripUnderscore)
     return true;
   if (NoStripUnderscore)
     return false;
   // If none of them are set, use the default value for platform.
   // macho has symbols prefix with "_" so strip by default.
   return Triple(sys::getProcessTriple()).isOSBinFormatMachO();
 }
 
-static std::string demangle(llvm::raw_ostream &OS, const std::string &Mangled) {
+static std::string demangleWord(const std::string &Mangled) {
   int Status;
+  std::string Prefix;
 
   const char *DecoratedStr = Mangled.c_str();
   if (shouldStripUnderscore())
     if (DecoratedStr[0] == '_')
       ++DecoratedStr;
   size_t DecoratedLength = strlen(DecoratedStr);
 
   char *Undecorated = nullptr;
 
   if (Types ||
       ((DecoratedLength >= 2 && strncmp(DecoratedStr, "_Z", 2) == 0) ||
        (DecoratedLength >= 4 && strncmp(DecoratedStr, "___Z", 4) == 0)))
     Undecorated = itaniumDemangle(DecoratedStr, nullptr, nullptr, &Status);
 
   if (!Undecorated &&
       (DecoratedLength > 6 && strncmp(DecoratedStr, "__imp_", 6) == 0)) {
-    OS << "import thunk for ";
+    Prefix = "import thunk for ";
     Undecorated = itaniumDemangle(DecoratedStr + 6, nullptr, nullptr, &Status);
   }
 
-  std::string Result(Undecorated ? Undecorated : Mangled);
+  std::string Result(Undecorated ? Prefix + Undecorated : Mangled);
   free(Undecorated);
   return Result;
 }
 
 // Split 'Source' on any character that fails to pass 'IsLegalChar'.  The
 // returned vector consists of pairs where 'first' is the delimited word, and
 // 'second' are the delimiters following that word.
 static void SplitStringDelims(

 // word is demangled.  Otherwise, the entire string is treated as a single
 // mangled item.  The result is output to 'OS'.
 static void demangleLine(llvm::raw_ostream &OS, StringRef Mangled, bool Split) {
   std::string Result;
   if (Split) {
     SmallVector<std::pair<StringRef, StringRef>, 16> Words;
     SplitStringDelims(Mangled, Words, IsLegalItaniumChar);
     for (const auto &Word : Words)
-      Result += demangle(OS, Word.first) + Word.second.str();
+      Result += demangleWord(Word.first) + Word.second.str();
   } else
-    Result = demangle(OS, Mangled);
+    Result = demangleWord(Mangled);
   OS << Result << '\n';
   OS.flush();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   cl::ParseCommandLineOptions(argc, argv, "llvm symbol undecoration tool\n");