[PATCH] D71425: [llvm-cxxfilt] Correctly demangle COFF import thunk

[Steven Wu via Phabricator via llvm-commits]

steven_wu marked 4 inline comments as done.
steven_wu added inline comments.


================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:4
 
-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
----------------
jhenderson wrote:
> 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.
I don't want to reformat the other test cases now. 


================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:5
+# This should demangle to origin string
+RUN: llvm-cxxfilt -n __imp__foo | FileCheck %s --check-prefix=CHECK-STRING
+
----------------
jhenderson wrote:
> 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?
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. 


================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:7-8
+
+RUN: llvm-cxxfilt -n __imp__ZSt6futureIvE __imp__ZSt6futureIvE | \
+RUN:    FileCheck %s --check-prefix=CHECK-SPLIT
 
----------------
jhenderson wrote:
> jhenderson wrote:
> > This isn't a "split" demangling as you call it. This is two separate strings ("__imp__ZSt6futureIvE" and "__imp__ZSt6futureIvE"), 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.
> > ("impZSt6futureIvE" and "impZSt6futureIvE")
> 
> This should say ("`__imp__ZSt6futureIvE`" and "`__imp__ZSt6futureIvE`") but Phabricator refrormatted it...
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. 


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:77
 
-static std::string demangle(llvm::raw_ostream &OS, const std::string &Mangled) {
+static std::string demangleWord(const std::string &Mangled) {
   int Status;
----------------
jhenderson wrote:
> Why did you change this name?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71425/new/

https://reviews.llvm.org/D71425