[PATCH] D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/readelf-s-alias.test:4-5
+RUN:   | FileCheck %s -check-prefix SEC
+RUN: llvm-readobj --sections %p/Inputs/trivial.obj.elf-i386 \
+RUN:   | FileCheck %s -check-prefix SEC
+
----------------
rupprecht wrote:
> jhenderson wrote:
> > Since -s should be an alias for --sections, maybe it's worth piping the -s output to a file and then comparing the output against --sections output.
> > 
> > Similar comment below for --symbols.
> > 
> > I assume we have sufficient separate testing for --symbols and --sections, so we can rely on those to show that we dump sections and symbols output correctly with the long-form switches. If we don't, we should add them.
> > Since -s should be an alias for --sections, maybe it's worth piping the -s output to a file and then comparing the output against --sections output.
> > 
> > Similar comment below for --symbols.
> Sorry, I don't understand this comment. This test is already running through FileCheck to make sure that -s and --sections both look like section output (for readobj) and similar for --symbols. Piping to a file and then comparing both files seems like it should pass, but is not really a targeted test -- the error would be "files differ" instead of "'Sections [' not found".
> 
> > 
> > I assume we have sufficient separate testing for --symbols and --sections, so we can rely on those to show that we dump sections and symbols output correctly with the long-form switches. If we don't, we should add them.
> The tests use short symbols by default. I flipped it so by default we use --symbols and --sections, with just a quick test that the aliases work.
I would say that we want -s and --sections/--symbols to be identical output. Technically, all this test does is show that -s/--sections/--symbols produces sections/symbols output. The test would actually pass if -s did both, which clearly isn't the intent of the test.

We don't need to show that --sections/--symbols produces sections/symbols output in this test. There's already a dedicated test for that. If you want to show that -s is truly an alias, then the whole output needs to be identical, and therefore "files differ" is actually the appropriate failure.

The main --sections/--symbols tests can test that -s prints sections/symbols output as well, if desired.


================
Comment at: test/tools/llvm-readobj/symbols.test:13
+RUN:   | FileCheck %s -check-prefix ELF
+
 COFF:      Symbols [
----------------
Maybe worth doing "llvm-readelf -s" here too


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:68
 
   // -file-headers, -h
   cl::opt<bool> FileHeaders("file-headers",
----------------
These comments should probably mention all of the public aliases, not just one long and one short.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:82
+                               cl::aliasopt(Sections), cl::NotHidden);
+  cl::alias SectionHeadersAlias("section-headers",
+                                cl::desc("Alias for --sections"),
----------------
This should be the "main" option, in my opinion, with the other two as aliases. Also, the help text for it should be "Display all section headers." since it doesn't display section contents.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:126
     cl::desc("Display the symbol table"));
-  cl::alias SymbolsShort("t",
-    cl::desc("Alias for --symbols"),
-    cl::aliasopt(Symbols));
+  cl::alias SymbolsMedium("syms", cl::desc("Alias for --symbols"),
+                          cl::aliasopt(Symbols));
----------------
Maybe SymbolsGNU would be clearer as to why this exists? Ditto dyn-syms below.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:618-619
+
+  // Only register -t in llvm-readobj, and readelf reserves it for
+  // --section-details (not implemented yet)
+  static cl::alias SymbolsShort("t", cl::desc("Alias for --symbols"),
----------------
and -> as
Missing trailing full stop.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:57
+              "--sections, --symbols, --relocations, --dynamic-table, --notes, "
+              "--version-info --unwind, --section-groups and --histogram."));
+  cl::alias AllShort("a", cl::desc("Alias for --all"), cl::aliasopt(All));
----------------
jhenderson wrote:
> Missing comma between --version-info and --unwind. Also --histogram isn't a thing in llvm-readelf, as far as I can tell (but you should probably add it in this patch).
--sections -> --section-headers
--histogram -> --elf-hash-histogram

--sections to me implies section headers and section contents, which isn't the case.


Repository:
  rL LLVM

https://reviews.llvm.org/D54124