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

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 7 inline comments as done.
jhenderson 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
+
----------------
grimar wrote:
> 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.
That's actually four tool calls (llvm-readobj x 2 + FileCheck x 2). This approach is the same number of calls (llvm-readobj x 2 + FileCheck + diff). I don't know, but I wouldn't be surprised if diff is as fast as the second FileCheck run, although I see your point about creating the extra files (although I doubt it makes more than a small fraction of a second difference).

I personally have a marginal preference for this approach overall, as I think it's a little easier to read, and has the potential to catch odd bugs, but I don't feel strongly about it, and would change it to progress the review. However, a more important issue is consistency. Due to the bug in llvm-readelf mentioned in the test below, the GNU output has to use a single file across multiple runs (or to run FileCheck twice). As that already pipes to a file, keeping the LLVM ones piping to a file helps keep things consistent.


================
Comment at: test/tools/llvm-readobj/demangle.test:26
+
+# LLVMCOMMON: Relocations [
+# LLVMCOMMON:      Section {{.*}} .rela.text.foo {
----------------
rupprecht wrote:
> This is a really annoying nit, but could you indent the expected side so it reads more easily, vs being indented based on how long the check prefix is? e.g. for this block:
> 
> ```
> # LLVMCOMMON:       xRelocations [
> # LLVMCOMMON:       x  Section {{.*}} .rela.text.foo {
> # LLVMDEMANGLE-NEXT:x    {{ }}foo(char){{ }}
> # LLVMMANGLED-NEXT: x    {{ }}_Z3fooc{{ }}
> # LLVMCOMMON-NEXT:  x  }
> # LLVMCOMMON:       x]
> ```
> (x's for clarity, showing the margin)
Sure. I'm the king of nit-picking, so feel free to pick at them however minor they may seem!


================
Comment at: test/tools/llvm-readobj/demangle.test:67
+# LLVMMANGLED-NEXT:  Sym: _Z4blahf{{ }}
+# LLVMCOMMON-NEXT: ]
+
----------------
grimar wrote:
> 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).
Whilst for the Addrsig bit, it's not too bad, I and a colleague found it much less readable for things like the relocation dumps. It also is less maintainable if we ever change the format slightly, as it requires updating two different places. Finally, it is harder to see where the actual differences are if written as you've suggested.

I think with the fix suggested by @rupprecht, and one small tweak (adding a '-' between LLVM/GNU and COMMON etc.) it is a little clearer and easier to distinguish the prefixes.


================
Comment at: test/tools/llvm-readobj/demangle.test:206
+    Content: "01000000020000002000000000000000"
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
----------------
grimar wrote:
> Does not seem you need `.llvm_addrsig` or `.llvm.call-graph-profile`? Could you reduce the yaml to keep only important parts?
They are needed to test the demangling of symbols in those two dumps. If they aren't in the yaml, they won't be tested (and they go through a separate call path to the other symbol names, so it's not like we're duplicating coverage).


================
Comment at: test/tools/llvm-readobj/demangle.test:218
+      Section: .text.foo
+ProgramHeaders:
+  - Type:  PT_LOAD
----------------
grimar wrote:
> The same, seems you do not need it.
llvm-readobj looks up dynamic data from the PT_DYNAMIC segment, not the .dynamic section, and uses that to dump dynamic relocations (who have a separate method for printing to static relocations, so need testing separately). The address information is dependent on the PT_LOAD the sections are in, hence we need both segments.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:191
+  cl::alias DemangleShort("C", cl::desc("Alias for --demangle"),
+                          cl::aliasopt(Demangle), cl::NotHidden);
+
----------------
grimar wrote:
> 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?
I agree it's inconsistent, and I'd like to improve that, but I'd actually lean towards having short options in the main help text, not just help-hidden. If you see a usage of "-C" (or whatever) in a build script, and it's not visible in the help text, it could be confusing for users unfamiliar with the tool. Putting it under -help-hidden doesn't help, because this is not a normal (outside LLVM) way of getting short alias information.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56791