[PATCH] D56791: [llvm-readobj][ELF]Add demangling support

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/demangle.test:14
+# RUN: FileCheck %s --input-file %t.llvm.long --check-prefixes=LLVMCOMMON,LLVMDEMANGLE
+# RUN: diff %t.llvm.long %t.llvm.short
+
----------------
I already saw such way (using 'diff' tool) just recently somewhere in LLVM mails,
but I am not sure what is the benefit of doing that? I mean isn't it easier to do in the usual way:

```
# RUN: llvm-readobj --symbols --relocations --dyn-symbols --dyn-relocations \
# RUN:              --elf-section-groups --elf-cg-profile --addrsig         \
# RUN:              --demangle %t.so | FileCheck %s --check-prefixes=LLVMCOMMON,LLVMDEMANGLE
# RUN: llvm-readobj --symbols --relocations --dyn-symbols --dyn-relocations \
# RUN:              --elf-section-groups --elf-cg-profile --addrsig         \
# RUN:              -C --demangle %t.so | FileCheck %s --check-prefixes=LLVMCOMMON,LLVMDEMANGLE
```

That would avoid creating 2 files and calling the diff. And since calling tools and creating files affect on a
speed of the test execution, I think it is good to try to reduce it.

I am not sure it is really worth to compare the whole output between the option and its alias.
I just think it is not common and a bit excessive.


================
Comment at: test/tools/llvm-readobj/demangle.test:67
+# LLVMMANGLED-NEXT:  Sym: _Z4blahf{{ }}
+# LLVMCOMMON-NEXT: ]
+
----------------
I would suggest to remove `LLVMCOMMON` and just use `LLVMDEMANGLE`/`LLVMMANGLED`, something like:


```
...
# LLVMDEMANGLE:           Addrsig [
# LLVMDEMANGLE-NEXT:   Sym: foo(char){{ }}
# LLVMDEMANGLE-NEXT:   Sym: blah(float){{ }}
# LLVMDEMANGLE-NEXT:  ]
...

...
# LLVMMANGLED:           Addrsig [
# LLVMMANGLED-NEXT:   Sym: foo(char){{ }}
# LLVMMANGLED-NEXT:   Sym: blah(float){{ }}
# LLVMMANGLED-NEXT:  ]
...
```

(I think introducing 3 tags for a FileCheck call is unnecessary overcomplication here, it should look and read simpler if you have only 2).


================
Comment at: test/tools/llvm-readobj/demangle.test:206
+    Content: "01000000020000002000000000000000"
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
----------------
Does not seem you need `.llvm_addrsig` or `.llvm.call-graph-profile`? Could you reduce the yaml to keep only important parts?


================
Comment at: test/tools/llvm-readobj/demangle.test:218
+      Section: .text.foo
+ProgramHeaders:
+  - Type:  PT_LOAD
----------------
The same, seems you do not need it.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:191
+  cl::alias DemangleShort("C", cl::desc("Alias for --demangle"),
+                          cl::aliasopt(Demangle), cl::NotHidden);
+
----------------
I noticed a small inconsistency in `llvm-objdump` about printing/not-printing the aliased with `-help`.
Seems they should only be printed witch `-help-hidden`, but atm two of them are always printed.
I was going to suggest a patch for this.

Seems you should not use `cl::NotHidden` here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56791