[PATCH] D113106: demangle xcoff label symbol for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/demangle.test:17
+
+# NM-DEMANGLE:      00000000 t
+# NM-DEMANGLE-NEXT: 00000040 T .func1(int)
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I'd suggest doing one of the following:
> > 1) adding --match-full-lines to your FileCheck command here
> > 2) omitting the symbol value and adding `{{$}}` to the end of the symbol (omitting the symbol value removes noise from the test).
> > 
> > This will prevent this line accidentally matching another symbol with a name. It will also ensure the demangling output looks right on other lines.
>  there is no symbol value in the test . it is symbol address.
"Symbol value" and "symbol address" are usually synonyms in other file formats. llvm-nm by default dumps three columns, "symbol value/address" being the first, and is the one I was referring to.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/demangle.test:20
+# NM-DEMANGLE-NEXT: 00000000 t .func0()
+# NM-DEMANGLE-NEXT: 00000120 b .bss
+# NM-DEMANGLE-NEXT: 00000100 d .data
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This test is purely about demangling functionality. Aside from having a single test-case where a symbol isn't demangled, I don't think you need all these other cases that don't demangle.
> > > 
> > > Similarly, you can omit most of the test cases where demangling does occur, as long as you have one per code path.
> > > 
> > > I think the following set of test cases is all you should have - ignore the other symbols (or more preferably just rebase this patch on top of @Esme's yaml2obj work, so that you can use yaml2obj to generate exactly the right set of symbols):
> > > # Empty name
> > > # Name consisting solely of `.`
> > > # Name starting with `.` that can't be demangled
> > > # Name starting with `.` that can be demangled
> > > # Name not starting with `.` that can't be demangled
> > > # Name not starting with `.` that can be demangled
> > 1. as I mentioned before, yaml2obj do not support an empty symbol.
> > 2. I will reuse the test_xlclang.o which provided in the  https://reviews.llvm.org/D112450 to test empty symbol
> > 3. when yam22obj, I will create to delete the test and add the empty name to test demangle_sym_name.test
> sorry , It should be
> 3. when yam2obj support empty symbol, I will create a patch to delete the test and add the empty name to test demangle_sym_name.test
> as I mentioned before, yaml2obj do not support an empty symbol.

Then add support for it? I can't imagine it would be hard to do. Alternatively, does it fail if you explicitly do something like `Name: ''`? I believe that's how we provide empty names in cases in other formats.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/demangle_sym_name.test:2
+## Test llvm-nm demangling of symbols for XCOFF object files.
+# RUN: yaml2obj --docnum=1 %s -o %t_yamlgen.o
+# RUN: llvm-nm --demangle  %t_yamlgen.o 2>&1 | FileCheck %s --check-prefix=NM-DEMANGLE
----------------
Simplify the name.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/demangle_sym_name.test:3
+# RUN: yaml2obj --docnum=1 %s -o %t_yamlgen.o
+# RUN: llvm-nm --demangle  %t_yamlgen.o 2>&1 | FileCheck %s --check-prefix=NM-DEMANGLE
+
----------------
I'd recommend adding `--match-full-lines` here too, to show that the name demangling has worked properly, and doesn't print some trailing string/garbage after the demangled name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113106