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

[Sid Manning via Phabricator via llvm-commits]

sidneym marked 3 inline comments as done.
sidneym added inline comments.


================
Comment at: test/tools/llvm-readobj/justheader.test:1
+RUN: llvm-readelf -e %p/Inputs/trivial.obj.elf-i386 \
+RUN:   | FileCheck %s
----------------
jhenderson wrote:
> 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.
Yes, I like the name "headers.test" better.

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident.  Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified.  Is that what you want?

The option -e is an alias for --headers so the output will match unless the meaning of one of the options is changed.

I used "all.test" as my template reasoning that -e is "almost all".




================
Comment at: test/tools/llvm-readobj/justheader.test:9-10
+CHECK: Program Headers:
+CHECK-NOT: Relocation section
+CHECK-NOT: Symbol table
+
----------------
jhenderson wrote:
> 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.
I'll add a new pass for negative testing.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:62
+  cl::opt<bool>
+      JustHeaders("headers",
+          cl::desc("Equivalent to setting: --file-headers, --program-headers, "
----------------
jhenderson wrote:
> 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?
Sounds good.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55298