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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.

Thanks for the answers, James. LGTM.



================
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
+
----------------
jhenderson wrote:
> 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.
> (although I doubt it makes more than a small fraction of a second difference).

Yes, but check-llvm already might take minutes to finish :) More tests - more slowdown.

I am OK with it as is.


================
Comment at: test/tools/llvm-readobj/demangle.test:16
+
+## Check that default is no demangling
+# RUN: llvm-readobj --symbols --relocations --dyn-symbols --dyn-relocations \
----------------
nit: full stop is missing.


================
Comment at: test/tools/llvm-readobj/demangle.test:206
+    Content: "01000000020000002000000000000000"
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
----------------
jhenderson wrote:
> 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).
Oops, indeed I did not notice you are using them. Sorry.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:191
+  cl::alias DemangleShort("C", cl::desc("Alias for --demangle"),
+                          cl::aliasopt(Demangle), cl::NotHidden);
+
----------------
jhenderson wrote:
> 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.
That sounds good. I was not sure what the way will be preferred (I supposed hiding aliases was intentional), but for me, it would be more convenient to see them under `-help` (that was why I found this initially, I did not see the lines I would expect to see, but saw the other lines at the same time).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56791