[PATCH] D55298: [llvm-readelf] Add -e/--headers support to readobj/elf

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Please remember to include the context when creating the diff with Phabricator (see the section starting "To get a full diff" here <https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface>.



================
Comment at: test/tools/llvm-readobj/justheader.test:1
+RUN: llvm-readelf -e %p/Inputs/trivial.obj.elf-i386 \
+RUN:   | FileCheck %s
----------------
The test needs renaming in line with the option name. I'd just suggest calling it "headers.test" since that is the option name.

I also think it would be a good idea to test that --headers produces the same headers for both llvm-readelf and llvm-readobj. In other cases (I'm thinking --program-headers specifically), the two produce different output, and not just in terms of style.

Also, rather than running FileCheck on both outputs, I personally would prefer for the second test case to compare its output to the first case to show that the --headers output is identical to -e. You should still run FileCheck on the first one, but you will need to run it using --input-file instead of stdin instead.

Finally, I'm not sure either way as to whether you should be checking more of the output (e.g. the contents of the ELF header, section headers etc). I can be persuaded either way, but I'd like to hear your justification, please.


================
Comment at: test/tools/llvm-readobj/justheader.test:9-10
+CHECK: Program Headers:
+CHECK-NOT: Relocation section
+CHECK-NOT: Symbol table
+
----------------
You may not be aware of this, but CHECK-NOT only checks the area between the previous and next matches, so in this case, it will only check that no relocation section or symbol table are dumped at the end. They could be printed e.g. between the section headers and program headers, and you wouldn't know.


================
Comment at: test/tools/llvm-readobj/justheader.test:11
+CHECK-NOT: Symbol table
+
----------------
Nit: Did you mean to have a blank line at the end of this file?


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:62
+  cl::opt<bool>
+      JustHeaders("headers",
+          cl::desc("Equivalent to setting: --file-headers, --program-headers, "
----------------
rupprecht wrote:
> nit: "JustHeaders" kind of sounds like this will only print headers and nothing else, but "llvm-readelf -e -n" should still print notes in addition to headers (for example). Maybe "AllHeaders" would be a better name, and hopefully not confusing with the "All" option above?
Why not simply "Headers", since that is what the option is called?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55298